From 7aa0bd7211d9cc1aeed1650d56907a0f2e1ad6c8 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Wed, 9 Oct 2024 15:41:49 +0200 Subject: [PATCH 01/16] move 'assume yes' cli option under individual content commands, added cli delete/remove option, scaffolded functionality for this --- crates/rari-cli/main.rs | 37 +++- crates/rari-cli/serve.rs | 6 +- crates/rari-doc/src/helpers/subpages.rs | 5 + crates/rari-tools/src/lib.rs | 2 + crates/rari-tools/src/move.rs | 18 +- crates/rari-tools/src/remove.rs | 210 +++++++++++++++++++ crates/rari-tools/src/tests/fixtures/docs.rs | 4 +- crates/rari-tools/src/utils.rs | 12 ++ 8 files changed, 271 insertions(+), 23 deletions(-) create mode 100644 crates/rari-tools/src/remove.rs create mode 100644 crates/rari-tools/src/utils.rs diff --git a/crates/rari-cli/main.rs b/crates/rari-cli/main.rs index 8ced3ce8..b729e534 100644 --- a/crates/rari-cli/main.rs +++ b/crates/rari-cli/main.rs @@ -21,6 +21,7 @@ use rari_doc::utils::TEMPL_RECORDER_SENDER; use rari_tools::history::gather_history; use rari_tools::popularities::update_popularities; use rari_tools::r#move::r#move; +use rari_tools::remove::remove; use rari_types::globals::{build_out_root, content_root, content_translated_root, SETTINGS}; use rari_types::settings::Settings; use self_update::cargo_crate_version; @@ -44,8 +45,6 @@ struct Cli { no_cache: bool, #[arg(long)] skip_updates: bool, - #[arg(short = 'y', long, help = "Assume yes to all prompts")] - assume_yes: bool, #[command(subcommand)] command: Commands, } @@ -66,6 +65,7 @@ enum Commands { enum ContentSubcommand { /// Moves content from one slug to another Move(MoveArgs), + Delete(DeleteArgs), } #[derive(Args)] @@ -73,6 +73,20 @@ struct MoveArgs { old_slug: String, new_slug: String, locale: Option, + #[arg(short = 'y', long, help = "Assume yes to all prompts")] + assume_yes: bool, +} + +#[derive(Args)] +struct DeleteArgs { + slug: String, + locale: Option, + #[arg(short, long, default_value_t = false)] + recursive: bool, + #[arg(long)] + redirect: Option, + #[arg(short = 'y', long, help = "Assume yes to all prompts")] + assume_yes: bool, } #[derive(Args)] @@ -331,7 +345,24 @@ fn main() -> Result<(), Error> { &args.old_slug, &args.new_slug, args.locale.as_deref(), - cli.assume_yes, + args.assume_yes, + )?; + } + ContentSubcommand::Delete(args) => { + // slug: String, + // locale: Option, + // #[arg(short, long, default_value_t = false)] + // recursive: bool, + // #[arg(long)] + // redirect: Option, + // #[arg(short = 'y', long, help = "Assume yes to all prompts")] + // assume_yes: bool, + remove( + &args.slug, + args.locale.as_deref(), + args.recursive, + args.redirect.as_deref(), + args.assume_yes, )?; } }, diff --git a/crates/rari-cli/serve.rs b/crates/rari-cli/serve.rs index eb2faa2e..3c878e71 100644 --- a/crates/rari-cli/serve.rs +++ b/crates/rari-cli/serve.rs @@ -26,11 +26,7 @@ struct AppError(anyhow::Error); impl IntoResponse for AppError { fn into_response(self) -> Response { - ( - StatusCode::INTERNAL_SERVER_ERROR, - error!("🤷‍♂️: {}", self.0), - ) - .into_response() + (StatusCode::INTERNAL_SERVER_ERROR, error!("🤷‍♂️: {}", self.0)).into_response() } } impl From for AppError diff --git a/crates/rari-doc/src/helpers/subpages.rs b/crates/rari-doc/src/helpers/subpages.rs index 947bfb5a..d59053c1 100644 --- a/crates/rari-doc/src/helpers/subpages.rs +++ b/crates/rari-doc/src/helpers/subpages.rs @@ -235,6 +235,11 @@ fn read_sub_folders_cached( .collect()) } +// This is only used in tests external to this crate +pub fn clear_subfolder_cache() { + memoized_flush_read_sub_folders_cached(); +} + fn split_into_parts(s: &str) -> Vec<(bool, &str)> { let mut parts = Vec::new(); let mut start = 0; diff --git a/crates/rari-tools/src/lib.rs b/crates/rari-tools/src/lib.rs index fbbc2be3..d1045eea 100644 --- a/crates/rari-tools/src/lib.rs +++ b/crates/rari-tools/src/lib.rs @@ -3,6 +3,8 @@ pub mod history; pub mod r#move; pub mod popularities; pub mod redirects; +pub mod remove; #[cfg(test)] pub mod tests; +mod utils; pub mod wikihistory; diff --git a/crates/rari-tools/src/move.rs b/crates/rari-tools/src/move.rs index a62c0877..e1bc6149 100644 --- a/crates/rari-tools/src/move.rs +++ b/crates/rari-tools/src/move.rs @@ -19,6 +19,7 @@ use rari_types::locale::Locale; use crate::error::ToolError; use crate::redirects::add_redirects; +use crate::utils::parent_slug; use crate::wikihistory::update_wiki_history; pub fn r#move( @@ -214,15 +215,6 @@ fn do_move( Ok(pairs) } -fn parent_slug(slug: &str) -> Result<&str, ToolError> { - let slug = slug.trim_end_matches('/'); - if let Some(i) = slug.rfind('/') { - Ok(&slug[..i]) - } else { - Err(ToolError::InvalidSlug(Cow::Borrowed("slug has no parent"))) - } -} - fn validate_args(old_slug: &str, new_slug: &str) -> Result<(), ToolError> { if old_slug.is_empty() { return Err(ToolError::InvalidSlug(Cow::Borrowed( @@ -247,15 +239,13 @@ fn validate_args(old_slug: &str, new_slug: &str) -> Result<(), ToolError> { Ok(()) } -#[cfg(test)] -use serial_test::file_serial; - // These tests use file system fixtures to simulate content and translated content. // The file system is a shared resource, so we force tests to be run serially, // to avoid concurrent fixture management issues. // Using `file_serial` as a synchonization lock, we should be able to run all tests -// using the same `key` to be serialized across modules. - +// using the same `key` (here: file_fixtures) to be serialized across modules. +#[cfg(test)] +use serial_test::file_serial; #[cfg(test)] #[file_serial(file_fixtures)] mod test { diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs new file mode 100644 index 00000000..0fc4697f --- /dev/null +++ b/crates/rari-tools/src/remove.rs @@ -0,0 +1,210 @@ +use std::borrow::Cow; +use std::str::FromStr; + +use console::{style, Style}; +use rari_doc::helpers::subpages::get_sub_pages; +use rari_doc::pages::page::{self, PageCategory, PageLike}; +use rari_doc::resolve::build_url; +use rari_types::locale::Locale; + +use crate::error::ToolError; +use crate::utils::parent_slug; + +pub fn remove( + slug: &str, + locale: Option<&str>, + recursive: bool, + redirect: Option<&str>, + _assume_yes: bool, +) -> Result<(), ToolError> { + validate_args(slug)?; + let locale = if let Some(l) = locale { + Locale::from_str(l)? + } else { + Locale::default() + }; + + let green = Style::new().green(); + let _red = Style::new().red(); + let _bold = Style::new().bold(); + let changes = do_remove(slug, locale, recursive, redirect, true)?; + if changes.is_empty() { + println!("{}", green.apply_to("No changes would be made")); + return Ok(()); + } else { + return Ok(()); + } + // Ok(()) +} + +fn do_remove( + slug: &str, + locale: Locale, + recursive: bool, + redirect: Option<&str>, + dry_run: bool, +) -> Result, ToolError> { + let url = build_url(slug, locale, PageCategory::Doc)?; + let doc = page::Page::from_url(&url)?; + let real_slug = doc.slug(); + + // If we get a redirect value passed in, it is either a slug or a complete url. + // If it is a slug, check if it actually exists, otherwise bail. + let redirect_value = if let Some(redirect_str) = redirect { + let ret = if !redirect_str.starts_with("http") { + let redirect_url = build_url(redirect_str, locale, PageCategory::Doc)?; + if !page::Page::exists(&redirect_url) { + return Err(ToolError::InvalidSlug(Cow::Owned(format!( + "redirect slug does not exist: {redirect_url}" + )))); + } + Some(redirect_url) + } else { + Some(redirect_str.to_owned()) + }; + ret + } else { + None + }; + + let subpages = get_sub_pages(&url, None, Default::default())?; + if !recursive && !subpages.is_empty() { + return Err(ToolError::InvalidSlug(Cow::Owned(format!( + "page has subpages, use --recursive to remove them: {real_slug}" + )))); + } + + // let new_parent_slug = parent_slug(slug)?; + // if !page::Page::exists(&build_url(new_parent_slug, locale, PageCategory::Doc)?) { + // return Err(ToolError::InvalidSlug(Cow::Owned(format!( + // "new parent slug does not exist: {new_parent_slug}" + // )))); + // } + // // let subpages = get_sub_pages(&old_url, None, Default::default())?; + + return Ok(vec![]); +} + +fn validate_args(slug: &str) -> Result<(), ToolError> { + if slug.is_empty() { + return Err(ToolError::InvalidSlug(Cow::Borrowed( + "old slug cannot be empty", + ))); + } + if slug.contains("#") { + return Err(ToolError::InvalidSlug(Cow::Borrowed( + "old slug cannot contain '#'", + ))); + } + Ok(()) +} + +// These tests use file system fixtures to simulate content and translated content. +// The file system is a shared resource, so we force tests to be run serially, +// to avoid concurrent fixture management issues. +// Using `file_serial` as a synchonization lock, we should be able to run all tests +// using the same `key` (here: file_fixtures) to be serialized across modules. +#[cfg(test)] +use serial_test::file_serial; +#[cfg(test)] +#[file_serial(file_fixtures)] +mod test { + + // use std::collections::HashMap; + // use std::path::Path; + + use super::*; + use crate::tests::fixtures::docs::DocFixtures; + // use crate::redirects; + // use crate::tests::fixtures::docs::DocFixtures; + // use crate::tests::fixtures::redirects::RedirectFixtures; + // use crate::tests::fixtures::wikihistory::WikihistoryFixtures; + + #[test] + fn test_validate_args() { + assert!(validate_args("old").is_ok()); + assert!(validate_args("").is_err()); + assert!(validate_args("old#").is_err()); + } + + #[test] + fn test_redirect_option() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleTwo".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + + // valid redirect + let result = do_remove( + "Web/API/ExampleTwo", + Locale::EnUs, + false, + Some("Web/API/ExampleOne"), + true, + ); + assert!(result.is_ok()); + + // invalid redirect + let result = do_remove( + "Web/API/ExampleTwo", + Locale::EnUs, + false, + Some("Web/API/ExampleNonExisting"), + true, + ); + assert!(result.is_err()); + + // external URL + let result = do_remove( + "Web/API/ExampleTwo", + Locale::EnUs, + false, + Some("https://example.com/"), + true, + ); + assert!(result.is_ok()); + + // no redirect + let result = do_remove("Web/API/ExampleTwo", Locale::EnUs, false, None, true); + assert!(result.is_ok()); + } + + #[test] + fn test_existing_subpages() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleOne/Subpage".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + + // no recursive, we bail because of existing subpages + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); + assert!(result.is_err()); + + // recursive + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, true, None, true); + assert!(result.is_ok()); + } + + #[test] + fn test_no_subpages() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + + // no recursive, but no subpages, so it is ok + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); + assert!(result.is_ok()); + + // recursive, all the same + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, true, None, true); + assert!(result.is_ok()); + } + + #[test] + fn test_nonexisting() { + // This does not exist + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); + assert!(result.is_err()); + } +} diff --git a/crates/rari-tools/src/tests/fixtures/docs.rs b/crates/rari-tools/src/tests/fixtures/docs.rs index d8b928a3..fb210660 100644 --- a/crates/rari-tools/src/tests/fixtures/docs.rs +++ b/crates/rari-tools/src/tests/fixtures/docs.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use fake::faker::lorem::en::Paragraph; use fake::Fake; use indoc::formatdoc; +use rari_doc::helpers::subpages::clear_subfolder_cache; use rari_doc::pages::page::PageCategory; use rari_doc::resolve::{build_url, url_meta_from, UrlMeta}; use rari_doc::utils::root_for_locale; @@ -26,8 +27,9 @@ impl DocFixtures { } fn new_internal(slugs: &[String], locale: Locale, do_not_remove: bool) -> Self { + // clear the subfolder cache + clear_subfolder_cache(); // create doc file for each slug in the vector, in the configured root directory for the locale - // Iterate over each slug and create a file in the root directory let _files: Vec = slugs .iter() diff --git a/crates/rari-tools/src/utils.rs b/crates/rari-tools/src/utils.rs new file mode 100644 index 00000000..dbb8b173 --- /dev/null +++ b/crates/rari-tools/src/utils.rs @@ -0,0 +1,12 @@ +use std::borrow::Cow; + +use crate::error::ToolError; + +pub(crate) fn parent_slug(slug: &str) -> Result<&str, ToolError> { + let slug = slug.trim_end_matches('/'); + if let Some(i) = slug.rfind('/') { + Ok(&slug[..i]) + } else { + Err(ToolError::InvalidSlug(Cow::Borrowed("slug has no parent"))) + } +} From 8c96336306236b10a22b738f7fce45f0bcdfae43 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Wed, 9 Oct 2024 16:47:18 +0200 Subject: [PATCH 02/16] make cache settings of get_subfolders configurable --- .cargo/config.toml | 1 + crates/rari-doc/src/helpers/subpages.rs | 19 +++++++----- crates/rari-tools/src/error.rs | 3 ++ crates/rari-tools/src/remove.rs | 31 ++++++++++++++------ crates/rari-tools/src/tests/fixtures/docs.rs | 3 -- crates/rari-types/src/settings.rs | 4 +++ 6 files changed, 41 insertions(+), 20 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 0bf84463..c52a0c72 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,3 +1,4 @@ [env] TESTING_CONTENT_ROOT = { value = "tests/data/content/files", relative = true } TESTING_CONTENT_TRANSLATED_ROOT = { value = "tests/data/translated_content/files", relative = true } +TESTING_CACHE_CONTENT = "0" diff --git a/crates/rari-doc/src/helpers/subpages.rs b/crates/rari-doc/src/helpers/subpages.rs index d59053c1..d1a53a7c 100644 --- a/crates/rari-doc/src/helpers/subpages.rs +++ b/crates/rari-doc/src/helpers/subpages.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use memoize::memoize; use rari_types::fm_types::{FeatureStatus, PageType}; -use rari_types::globals::deny_warnings; +use rari_types::globals::{cache_content, deny_warnings}; use rari_types::locale::Locale; use super::titles::api_page_title; @@ -207,7 +207,7 @@ pub fn get_sub_pages( let doc = Page::from_url(url)?; let full_path = doc.full_path(); if let Some(folder) = full_path.parent() { - let sub_folders = read_sub_folders_cached(folder.to_path_buf(), depth)?; + let sub_folders = read_sub_folders(folder.to_path_buf(), depth)?; let mut sub_pages = sub_folders .iter() @@ -220,9 +220,17 @@ pub fn get_sub_pages( Ok(vec![]) } +fn read_sub_folders(folder: PathBuf, depth: Option) -> Result, ignore::Error> { + if cache_content() { + read_sub_folders_internal(folder, depth) + } else { + memoized_original_read_sub_folders_internal(folder, depth) + } +} + #[memoize(SharedCache)] #[allow(non_snake_case)] -fn read_sub_folders_cached( +fn read_sub_folders_internal( folder: PathBuf, depth: Option, ) -> Result, ignore::Error> { @@ -235,11 +243,6 @@ fn read_sub_folders_cached( .collect()) } -// This is only used in tests external to this crate -pub fn clear_subfolder_cache() { - memoized_flush_read_sub_folders_cached(); -} - fn split_into_parts(s: &str) -> Vec<(bool, &str)> { let mut parts = Vec::new(); let mut start = 0; diff --git a/crates/rari-tools/src/error.rs b/crates/rari-tools/src/error.rs index ef45dbb9..23d99f75 100644 --- a/crates/rari-tools/src/error.rs +++ b/crates/rari-tools/src/error.rs @@ -41,6 +41,9 @@ pub enum ToolError { #[error(transparent)] RedirectError(#[from] RedirectError), + #[error("Page has subpages: {0}")] + HasSubpagesError(Cow<'static, str>), + #[error("Unknonwn error")] Unknown(&'static str), } diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 0fc4697f..5a8ba8ce 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -43,7 +43,7 @@ fn do_remove( recursive: bool, redirect: Option<&str>, dry_run: bool, -) -> Result, ToolError> { +) -> Result, ToolError> { let url = build_url(slug, locale, PageCategory::Doc)?; let doc = page::Page::from_url(&url)?; let real_slug = doc.slug(); @@ -51,27 +51,37 @@ fn do_remove( // If we get a redirect value passed in, it is either a slug or a complete url. // If it is a slug, check if it actually exists, otherwise bail. let redirect_value = if let Some(redirect_str) = redirect { - let ret = if !redirect_str.starts_with("http") { + if !redirect_str.starts_with("http") { let redirect_url = build_url(redirect_str, locale, PageCategory::Doc)?; if !page::Page::exists(&redirect_url) { - return Err(ToolError::InvalidSlug(Cow::Owned(format!( + return Err(ToolError::InvalidRedirectToURL(format!( "redirect slug does not exist: {redirect_url}" - )))); + ))); } Some(redirect_url) } else { Some(redirect_str.to_owned()) - }; - ret + } } else { None }; let subpages = get_sub_pages(&url, None, Default::default())?; if !recursive && !subpages.is_empty() { - return Err(ToolError::InvalidSlug(Cow::Owned(format!( - "page has subpages, use --recursive to remove them: {real_slug}" - )))); + return Err(ToolError::HasSubpagesError(Cow::Borrowed( + "page has subpages, use --recursive to delete recursively", + ))); + } + + let slugs_to_remove = [doc.clone()] + .iter() + .chain(&subpages) + .map(|page_ref| page_ref.slug().to_owned()) + .collect::>(); + + if dry_run { + // If there is a redirect, check if it is valid, otherwise bail. + return Ok(slugs_to_remove); } // let new_parent_slug = parent_slug(slug)?; @@ -153,6 +163,7 @@ mod test { Some("Web/API/ExampleNonExisting"), true, ); + assert!(matches!(result, Err(ToolError::InvalidRedirectToURL(_)))); assert!(result.is_err()); // external URL @@ -181,6 +192,7 @@ mod test { // no recursive, we bail because of existing subpages let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); assert!(result.is_err()); + assert!(matches!(result, Err(ToolError::HasSubpagesError(_)))); // recursive let result = do_remove("Web/API/ExampleOne", Locale::EnUs, true, None, true); @@ -205,6 +217,7 @@ mod test { fn test_nonexisting() { // This does not exist let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); + assert!(matches!(result, Err(ToolError::DocError(_)))); assert!(result.is_err()); } } diff --git a/crates/rari-tools/src/tests/fixtures/docs.rs b/crates/rari-tools/src/tests/fixtures/docs.rs index fb210660..900e0d7c 100644 --- a/crates/rari-tools/src/tests/fixtures/docs.rs +++ b/crates/rari-tools/src/tests/fixtures/docs.rs @@ -4,7 +4,6 @@ use std::path::PathBuf; use fake::faker::lorem::en::Paragraph; use fake::Fake; use indoc::formatdoc; -use rari_doc::helpers::subpages::clear_subfolder_cache; use rari_doc::pages::page::PageCategory; use rari_doc::resolve::{build_url, url_meta_from, UrlMeta}; use rari_doc::utils::root_for_locale; @@ -27,8 +26,6 @@ impl DocFixtures { } fn new_internal(slugs: &[String], locale: Locale, do_not_remove: bool) -> Self { - // clear the subfolder cache - clear_subfolder_cache(); // create doc file for each slug in the vector, in the configured root directory for the locale // Iterate over each slug and create a file in the root directory let _files: Vec = slugs diff --git a/crates/rari-types/src/settings.rs b/crates/rari-types/src/settings.rs index 1ef683d2..0dfa687b 100644 --- a/crates/rari-types/src/settings.rs +++ b/crates/rari-types/src/settings.rs @@ -53,6 +53,10 @@ impl Settings { "CONTENT_TRANSLATED_ROOT", std::env::var("TESTING_CONTENT_TRANSLATED_ROOT").unwrap(), ); + std::env::set_var( + "CACHE_CONTENT", + std::env::var("TESTING_CACHE_CONTENT").unwrap(), + ); Self::new_internal() } #[cfg(not(feature = "testing"))] From 79777fe2d881f5f4f9a5f078dfc0904e501966bf Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Wed, 9 Oct 2024 18:22:40 +0200 Subject: [PATCH 03/16] CLI info + logic --- crates/rari-tools/src/remove.rs | 61 ++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 5a8ba8ce..cb7966a9 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -2,6 +2,8 @@ use std::borrow::Cow; use std::str::FromStr; use console::{style, Style}; +use dialoguer::theme::ColorfulTheme; +use dialoguer::Confirm; use rari_doc::helpers::subpages::get_sub_pages; use rari_doc::pages::page::{self, PageCategory, PageLike}; use rari_doc::resolve::build_url; @@ -15,7 +17,7 @@ pub fn remove( locale: Option<&str>, recursive: bool, redirect: Option<&str>, - _assume_yes: bool, + assume_yes: bool, ) -> Result<(), ToolError> { validate_args(slug)?; let locale = if let Some(l) = locale { @@ -25,16 +27,55 @@ pub fn remove( }; let green = Style::new().green(); - let _red = Style::new().red(); - let _bold = Style::new().bold(); + let red = Style::new().red(); + let yellow = Style::new().yellow(); + let bold = Style::new().bold(); let changes = do_remove(slug, locale, recursive, redirect, true)?; if changes.is_empty() { println!("{}", green.apply_to("No changes would be made")); return Ok(()); } else { - return Ok(()); + println!( + "{} {} {}", + green.apply_to("This will delete"), + bold.apply_to(changes.len()), + green.apply_to("documents:"), + ); + for slug in changes { + println!("{}", red.apply_to(&slug)); + } + if let Some(redirect) = redirect { + println!( + "{} {} to: {}", + green.apply_to("Redirecting"), + green.apply_to(if recursive { + "each document" + } else { + "document" + }), + green.apply_to(&redirect), + ); + } else { + println!("{}", yellow.apply_to("Deleting without a redirect. Consider using the --redirect option with a related page instead.")); + } } - // Ok(()) + + if assume_yes + || Confirm::with_theme(&ColorfulTheme::default()) + .with_prompt("Proceed?") + .default(true) + .interact() + .unwrap_or_default() + { + let removed = do_remove(slug, locale, recursive, redirect, false)?; + println!( + "{} {} {}", + green.apply_to("Removed"), + bold.apply_to(removed.len()), + green.apply_to("documents"), + ); + } + Ok(()) } fn do_remove( @@ -68,9 +109,10 @@ fn do_remove( let subpages = get_sub_pages(&url, None, Default::default())?; if !recursive && !subpages.is_empty() { - return Err(ToolError::HasSubpagesError(Cow::Borrowed( - "page has subpages, use --recursive to delete recursively", - ))); + return Err(ToolError::HasSubpagesError(Cow::Owned(format!( + "{0}, use --recursive to delete recursively", + slug + )))); } let slugs_to_remove = [doc.clone()] @@ -80,10 +122,11 @@ fn do_remove( .collect::>(); if dry_run { - // If there is a redirect, check if it is valid, otherwise bail. return Ok(slugs_to_remove); } + let removed: Vec = Vec::new(); + // let new_parent_slug = parent_slug(slug)?; // if !page::Page::exists(&build_url(new_parent_slug, locale, PageCategory::Doc)?) { // return Err(ToolError::InvalidSlug(Cow::Owned(format!( From 450a7a5c119385a53ec83b86f616ee47b5c9aaac Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 10 Oct 2024 14:22:45 +0200 Subject: [PATCH 04/16] refactored wiki history management, more tests --- crates/rari-tools/src/move.rs | 23 +-- crates/rari-tools/src/remove.rs | 215 +++++++++++++++++++++++++-- crates/rari-tools/src/utils.rs | 25 ++++ crates/rari-tools/src/wikihistory.rs | 51 +++++-- 4 files changed, 264 insertions(+), 50 deletions(-) diff --git a/crates/rari-tools/src/move.rs b/crates/rari-tools/src/move.rs index e1bc6149..84dbcd5b 100644 --- a/crates/rari-tools/src/move.rs +++ b/crates/rari-tools/src/move.rs @@ -171,7 +171,7 @@ fn do_move( } // Conditional command for testing. In testing, we do not use git, because the test - // fixtures are no under git control. SO instead of `git mv …` we use `mv …`. + // fixtures are not under git control. Instead of `git mv …` we use `mv …`. let command = if cfg!(test) { "mv" } else { "git" }; let args = if cfg!(test) { vec![old_folder_path.as_os_str(), new_folder_path.as_os_str()] @@ -258,6 +258,7 @@ mod test { use crate::tests::fixtures::docs::DocFixtures; use crate::tests::fixtures::redirects::RedirectFixtures; use crate::tests::fixtures::wikihistory::WikihistoryFixtures; + use crate::utils::check_file_existence; fn s(s: &str) -> String { s.to_string() @@ -555,24 +556,4 @@ mod test { "/pt-BR/docs/Web/API/SomethingElse" ); } - - fn check_file_existence(root: &Path, should_exist: &[&str], should_not_exist: &[&str]) { - for relative_path in should_exist { - let parts = relative_path.split('/').collect::>(); - let mut path = PathBuf::from(root); - for part in parts { - path.push(part); - } - assert!(path.exists(), "{} should exist", path.display()); - } - - for relative_path in should_not_exist { - let parts = relative_path.split('/').collect::>(); - let mut path = PathBuf::from(root); - for part in parts { - path.push(part); - } - assert!(!path.exists(), "{} should not exist", path.display()); - } - } } diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index cb7966a9..21c408c1 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -1,4 +1,7 @@ use std::borrow::Cow; +use std::ffi::OsStr; +use std::path::PathBuf; +use std::process::Command; use std::str::FromStr; use console::{style, Style}; @@ -6,11 +9,13 @@ use dialoguer::theme::ColorfulTheme; use dialoguer::Confirm; use rari_doc::helpers::subpages::get_sub_pages; use rari_doc::pages::page::{self, PageCategory, PageLike}; -use rari_doc::resolve::build_url; +use rari_doc::resolve::{build_url, url_meta_from, UrlMeta}; +use rari_doc::utils::root_for_locale; use rari_types::locale::Locale; use crate::error::ToolError; use crate::utils::parent_slug; +use crate::wikihistory::{delete_from_wiki_history, update_wiki_history}; pub fn remove( slug: &str, @@ -70,10 +75,13 @@ pub fn remove( let removed = do_remove(slug, locale, recursive, redirect, false)?; println!( "{} {} {}", - green.apply_to("Removed"), + green.apply_to("Deleted"), bold.apply_to(removed.len()), green.apply_to("documents"), ); + + // Find references to deleted documents and + // list them for manual review } Ok(()) } @@ -91,7 +99,7 @@ fn do_remove( // If we get a redirect value passed in, it is either a slug or a complete url. // If it is a slug, check if it actually exists, otherwise bail. - let redirect_value = if let Some(redirect_str) = redirect { + let redirect_target = if let Some(redirect_str) = redirect { if !redirect_str.starts_with("http") { let redirect_url = build_url(redirect_str, locale, PageCategory::Doc)?; if !page::Page::exists(&redirect_url) { @@ -108,9 +116,9 @@ fn do_remove( }; let subpages = get_sub_pages(&url, None, Default::default())?; - if !recursive && !subpages.is_empty() { + if !recursive && !subpages.is_empty() && redirect.is_some() { return Err(ToolError::HasSubpagesError(Cow::Owned(format!( - "{0}, use --recursive to delete recursively", + "{0}, unable to remove and redirect a document with children", slug )))); } @@ -127,6 +135,78 @@ fn do_remove( let removed: Vec = Vec::new(); + // Remove the documents. For single documents, we just remove the `index.md` file and + // leave the folder structure in place. For recursive removal, we remove the entire + // folder structure, duplicating the original yari tool behaviour. + + // Conditional command for testing. In testing, we do not use git, because the test + // fixtures are not under git control. Instead of `git rm …` we use `rm …`. + let mut path = PathBuf::from(locale.as_folder_str()); + let url = build_url(real_slug, locale, PageCategory::Doc)?; + let UrlMeta { folder_path, .. } = url_meta_from(&url)?; + path.push(folder_path); + + let command = if cfg!(test) { "rm" } else { "git" }; + if recursive { + let args = if cfg!(test) { + vec![OsStr::new("-rf"), path.as_os_str()] + } else { + vec![OsStr::new("rm"), OsStr::new("-rf"), path.as_os_str()] + }; + + println!( + "recursive rm command: {} {}", + &command, + &args.join(OsStr::new(" ")).to_string_lossy() + ); + + // Execute the recursive remove command + let output = Command::new(command) + .args(args) + .current_dir(root_for_locale(locale)?) + .output() + .expect("failed to execute process"); + + if !output.status.success() { + return Err(ToolError::GitError(format!( + "Failed to remove files: {}", + String::from_utf8_lossy(&output.stderr) + ))); + } + } else { + path.push("index.md"); + let args = if cfg!(test) { + vec![path.as_os_str()] + } else { + vec![OsStr::new("rm"), &path.as_os_str()] + }; + + println!( + "single rm command: {} {}", + &command, + &args.join(OsStr::new(" ")).to_string_lossy() + ); + + // Execute the single file remove command + let output = Command::new(command) + .args(args) + .current_dir(root_for_locale(locale)?) + .output() + .expect("failed to execute process"); + + if !output.status.success() { + return Err(ToolError::GitError(format!( + "Failed to remove files: {}", + String::from_utf8_lossy(&output.stderr) + ))); + } + } + + // update the wiki history + delete_from_wiki_history(locale, &slugs_to_remove)?; + + // update the redirects map if needed + // let new_parent_slug = parent_slug(slug)?; // if !page::Page::exists(&build_url(new_parent_slug, locale, PageCategory::Doc)?) { // return Err(ToolError::InvalidSlug(Cow::Owned(format!( @@ -155,23 +235,19 @@ fn validate_args(slug: &str) -> Result<(), ToolError> { // These tests use file system fixtures to simulate content and translated content. // The file system is a shared resource, so we force tests to be run serially, // to avoid concurrent fixture management issues. -// Using `file_serial` as a synchonization lock, we should be able to run all tests -// using the same `key` (here: file_fixtures) to be serialized across modules. +// Using `file_serial` as a synchonization lock, we run all tests using +// the same `key` (here: file_fixtures) to be serialized across modules. #[cfg(test)] use serial_test::file_serial; #[cfg(test)] #[file_serial(file_fixtures)] mod test { - // use std::collections::HashMap; - // use std::path::Path; - use super::*; use crate::tests::fixtures::docs::DocFixtures; - // use crate::redirects; - // use crate::tests::fixtures::docs::DocFixtures; - // use crate::tests::fixtures::redirects::RedirectFixtures; - // use crate::tests::fixtures::wikihistory::WikihistoryFixtures; + use crate::tests::fixtures::wikihistory::WikihistoryFixtures; + use crate::utils::check_file_existence; + use crate::wikihistory::test_get_wiki_history; #[test] fn test_validate_args() { @@ -229,11 +305,22 @@ mod test { let slugs = vec![ "Web/API/ExampleOne".to_string(), "Web/API/ExampleOne/Subpage".to_string(), + "Web/API/RedirectTarget".to_string(), ]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); - // no recursive, we bail because of existing subpages + // no recursive, no redirect, ok even with subpages let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); + assert!(result.is_ok()); + + // no recursive, with redirect, not ok with subpages + let result = do_remove( + "Web/API/ExampleOne", + Locale::EnUs, + false, + Some("Web/API/RedirectTarget"), + true, + ); assert!(result.is_err()); assert!(matches!(result, Err(ToolError::HasSubpagesError(_)))); @@ -260,7 +347,103 @@ mod test { fn test_nonexisting() { // This does not exist let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, true); - assert!(matches!(result, Err(ToolError::DocError(_)))); assert!(result.is_err()); + assert!(matches!(result, Err(ToolError::DocError(_)))); + } + + #[test] + fn test_remove_single() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, false); + assert!(result.is_ok()); + + let should_exist = vec![]; + let should_not_exist = vec!["en-us/web/api/exampleone/index.md"]; + let root_path = root_for_locale(Locale::EnUs).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::EnUs); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + } + + #[test] + fn test_remove_single_with_redirect() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/RedirectTarget".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + + let result = do_remove( + "Web/API/ExampleOne", + Locale::EnUs, + false, + Some("Web/API/RedirectTarget"), + false, + ); + assert!(result.is_ok()); + + let should_exist = vec!["en-us/web/api/redirecttarget/index.md"]; + let should_not_exist = vec!["en-us/web/api/exampleone/index.md"]; + let root_path = root_for_locale(Locale::EnUs).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::EnUs); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + assert!(wiki_history.contains_key("Web/API/RedirectTarget")); + } + + #[test] + fn test_remove_single_with_subpages() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleOne/Subpage".to_string(), + "Web/API/RedirectTarget".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, false); + assert!(result.is_ok()); + + let should_exist = vec!["en-us/web/api/exampleone/subpage/index.md"]; + let should_not_exist = vec!["en-us/web/api/exampleone/index.md"]; + let root_path = root_for_locale(Locale::EnUs).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::EnUs); + // println!("{:?}", wiki_history); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + // assert!(wiki_history.contains_key("Web/API/ExampleOne/Subpage")); + assert!(wiki_history.contains_key("Web/API/RedirectTarget")); + } + + #[test] + fn test_remove_recursive() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleOne/Subpage".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + + let result = do_remove("Web/API/ExampleOne", Locale::EnUs, true, None, false); + assert!(result.is_ok()); + + let should_exist = vec![]; + let should_not_exist = vec![ + "en-us/web/api/exampleone/index.md", + "en-us/web/api/exampleone/subpage/index.md", + ]; + let root_path = root_for_locale(Locale::EnUs).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::EnUs); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + assert!(!wiki_history.contains_key("Web/API/ExampleOne/Subpage")); } } diff --git a/crates/rari-tools/src/utils.rs b/crates/rari-tools/src/utils.rs index dbb8b173..38f85657 100644 --- a/crates/rari-tools/src/utils.rs +++ b/crates/rari-tools/src/utils.rs @@ -10,3 +10,28 @@ pub(crate) fn parent_slug(slug: &str) -> Result<&str, ToolError> { Err(ToolError::InvalidSlug(Cow::Borrowed("slug has no parent"))) } } + +#[cfg(test)] +use std::path::Path; +#[cfg(test)] +pub(crate) fn check_file_existence(root: &Path, should_exist: &[&str], should_not_exist: &[&str]) { + use std::path::PathBuf; + + for relative_path in should_exist { + let parts = relative_path.split('/').collect::>(); + let mut path = PathBuf::from(root); + for part in parts { + path.push(part); + } + assert!(path.exists(), "{} should exist", path.display()); + } + + for relative_path in should_not_exist { + let parts = relative_path.split('/').collect::>(); + let mut path = PathBuf::from(root); + for part in parts { + path.push(part); + } + assert!(!path.exists(), "{} should not exist", path.display()); + } +} diff --git a/crates/rari-tools/src/wikihistory.rs b/crates/rari-tools/src/wikihistory.rs index a8fe827c..eec4d36e 100644 --- a/crates/rari-tools/src/wikihistory.rs +++ b/crates/rari-tools/src/wikihistory.rs @@ -10,30 +10,55 @@ use serde_json::Value; use crate::error::ToolError; pub fn update_wiki_history(locale: Locale, pairs: &Vec<(String, String)>) -> Result<(), ToolError> { - // Construct the path to "_wikihistory.json" - let locale_content_root = root_for_locale(locale)?; - let wiki_history_path = Path::new(locale_content_root) - .join(locale.as_folder_str()) - .join("_wikihistory.json"); - - // Read the content of the JSON file - let wiki_history_content = fs::read_to_string(&wiki_history_path)?; - - // Parse the JSON content into a BTreeMap (sorted map) - let mut all: BTreeMap = serde_json::from_str(&wiki_history_content)?; - + let mut all = read_wiki_history(locale)?; for (old_slug, new_slug) in pairs { if let Some(to) = all.remove(old_slug) { all.insert(new_slug.to_string(), to); } } + write_wiki_history(locale, all)?; + Ok(()) +} + +pub fn delete_from_wiki_history(locale: Locale, slugs: &Vec) -> Result<(), ToolError> { + let mut all = read_wiki_history(locale)?; + for slug in slugs { + all.remove(slug); + } + write_wiki_history(locale, all)?; + Ok(()) +} +fn write_wiki_history(locale: Locale, all: BTreeMap) -> Result<(), ToolError> { + let wiki_history_path = wiki_history_path(locale)?; let file = File::create(&wiki_history_path)?; let mut buffer = BufWriter::new(file); // Write the updated pretty JSON back to the file serde_json::to_writer_pretty(&mut buffer, &all)?; // Add a trailing newline buffer.write_all(b"\n")?; - Ok(()) } + +fn read_wiki_history(locale: Locale) -> Result, ToolError> { + let wiki_history_path = wiki_history_path(locale)?; + // Read the content of the JSON file + let wiki_history_content = fs::read_to_string(&wiki_history_path)?; + // Parse the JSON content into a BTreeMap (sorted map) + let all: BTreeMap = serde_json::from_str(&wiki_history_content)?; + Ok(all) +} + +fn wiki_history_path(locale: Locale) -> Result { + let locale_content_root = root_for_locale(locale)?; + Ok(Path::new(locale_content_root) + .join(locale.as_folder_str()) + .join("_wikihistory.json") + .to_string_lossy() + .to_string()) +} + +#[cfg(test)] +pub(crate) fn test_get_wiki_history(locale: Locale) -> BTreeMap { + read_wiki_history(locale).expect("Could not read wiki history") +} From 74463e69a30f107cc244bfc290176859fcbf3676 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 10 Oct 2024 15:02:36 +0200 Subject: [PATCH 05/16] fix history --- crates/rari-tools/src/remove.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 21c408c1..456559eb 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -123,11 +123,15 @@ fn do_remove( )))); } - let slugs_to_remove = [doc.clone()] - .iter() - .chain(&subpages) - .map(|page_ref| page_ref.slug().to_owned()) - .collect::>(); + let slugs_to_remove = if recursive { + [doc.clone()] + .iter() + .chain(&subpages) + .map(|page_ref| page_ref.slug().to_owned()) + .collect::>() + } else { + vec![real_slug.to_owned()] + }; if dry_run { return Ok(slugs_to_remove); @@ -418,7 +422,7 @@ mod test { let wiki_history = test_get_wiki_history(Locale::EnUs); // println!("{:?}", wiki_history); assert!(!wiki_history.contains_key("Web/API/ExampleOne")); - // assert!(wiki_history.contains_key("Web/API/ExampleOne/Subpage")); + assert!(wiki_history.contains_key("Web/API/ExampleOne/Subpage")); assert!(wiki_history.contains_key("Web/API/RedirectTarget")); } From b256e76a944985c453e146e1dd338acd4c654fe9 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 10 Oct 2024 17:17:41 +0200 Subject: [PATCH 06/16] utils refactored, redirects handling, tests --- crates/rari-tools/src/move.rs | 22 +---- crates/rari-tools/src/remove.rs | 158 ++++++++++++++++++++++++++------ crates/rari-tools/src/utils.rs | 58 ++++++++---- 3 files changed, 172 insertions(+), 66 deletions(-) diff --git a/crates/rari-tools/src/move.rs b/crates/rari-tools/src/move.rs index 84dbcd5b..a9cf18ee 100644 --- a/crates/rari-tools/src/move.rs +++ b/crates/rari-tools/src/move.rs @@ -250,15 +250,11 @@ use serial_test::file_serial; #[file_serial(file_fixtures)] mod test { - use std::collections::HashMap; - use std::path::Path; - use super::*; - use crate::redirects; use crate::tests::fixtures::docs::DocFixtures; use crate::tests::fixtures::redirects::RedirectFixtures; use crate::tests::fixtures::wikihistory::WikihistoryFixtures; - use crate::utils::check_file_existence; + use crate::utils::test_utils::{check_file_existence, get_redirects_map}; fn s(s: &str) -> String { s.to_string() @@ -404,13 +400,7 @@ mod test { check_file_existence(root_path, &should_exist, &should_not_exist); // check redirects - let mut redirects_path = PathBuf::from(root_path); - redirects_path.push(Locale::EnUs.as_folder_str()); - redirects_path.push("_redirects.txt"); - assert!(&redirects_path.exists()); - let mut redirects = HashMap::new(); - redirects::read_redirects_raw(&redirects_path, &mut redirects).unwrap(); - // println!("{:?}", redirects); + let redirects = get_redirects_map(Locale::EnUs); assert_eq!( redirects.get("/en-US/docs/Web/API/ExampleOne").unwrap(), "/en-US/docs/Web/API/ExampleOneNewLocation" @@ -520,13 +510,7 @@ mod test { check_file_existence(root_path, &should_exist, &should_not_exist); // check redirects - let mut redirects_path = PathBuf::from(root_path); - redirects_path.push(Locale::PtBr.as_folder_str()); - redirects_path.push("_redirects.txt"); - assert!(&redirects_path.exists()); - let mut redirects = HashMap::new(); - redirects::read_redirects_raw(&redirects_path, &mut redirects).unwrap(); - // println!("{:?}", redirects); + let redirects = get_redirects_map(Locale::PtBr); assert_eq!( redirects.get("/pt-BR/docs/Web/API/ExampleOne").unwrap(), "/pt-BR/docs/Web/API/ExampleOneNewLocation" diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 456559eb..3ae90b4d 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -4,7 +4,7 @@ use std::path::PathBuf; use std::process::Command; use std::str::FromStr; -use console::{style, Style}; +use console::Style; use dialoguer::theme::ColorfulTheme; use dialoguer::Confirm; use rari_doc::helpers::subpages::get_sub_pages; @@ -14,8 +14,8 @@ use rari_doc::utils::root_for_locale; use rari_types::locale::Locale; use crate::error::ToolError; -use crate::utils::parent_slug; -use crate::wikihistory::{delete_from_wiki_history, update_wiki_history}; +use crate::redirects::add_redirects; +use crate::wikihistory::delete_from_wiki_history; pub fn remove( slug: &str, @@ -137,8 +137,6 @@ fn do_remove( return Ok(slugs_to_remove); } - let removed: Vec = Vec::new(); - // Remove the documents. For single documents, we just remove the `index.md` file and // leave the folder structure in place. For recursive removal, we remove the entire // folder structure, duplicating the original yari tool behaviour. @@ -158,12 +156,6 @@ fn do_remove( vec![OsStr::new("rm"), OsStr::new("-rf"), path.as_os_str()] }; - println!( - "recursive rm command: {} {}", - &command, - &args.join(OsStr::new(" ")).to_string_lossy() - ); - // Execute the recursive remove command let output = Command::new(command) .args(args) @@ -185,12 +177,6 @@ fn do_remove( vec![OsStr::new("rm"), &path.as_os_str()] }; - println!( - "single rm command: {} {}", - &command, - &args.join(OsStr::new(" ")).to_string_lossy() - ); - // Execute the single file remove command let output = Command::new(command) .args(args) @@ -210,27 +196,32 @@ fn do_remove( delete_from_wiki_history(locale, &slugs_to_remove)?; // update the redirects map if needed + if let Some(new_slug) = redirect_target { + let pairs = slugs_to_remove + .iter() + .map(|slug| { + let old_url = build_url(slug, locale, PageCategory::Doc)?; + // let new_url = build_url(&new_slug, locale, PageCategory::Doc)?; + Ok((old_url, new_slug.to_owned())) + }) + .collect::, ToolError>>()?; + add_redirects(locale, &pairs)?; + } - // let new_parent_slug = parent_slug(slug)?; - // if !page::Page::exists(&build_url(new_parent_slug, locale, PageCategory::Doc)?) { - // return Err(ToolError::InvalidSlug(Cow::Owned(format!( - // "new parent slug does not exist: {new_parent_slug}" - // )))); - // } - // // let subpages = get_sub_pages(&old_url, None, Default::default())?; + // check references to the removed documents (do it in the main method) - return Ok(vec![]); + Ok(slugs_to_remove) } fn validate_args(slug: &str) -> Result<(), ToolError> { if slug.is_empty() { return Err(ToolError::InvalidSlug(Cow::Borrowed( - "old slug cannot be empty", + "slug cannot be empty", ))); } if slug.contains("#") { return Err(ToolError::InvalidSlug(Cow::Borrowed( - "old slug cannot contain '#'", + "slug cannot contain '#'", ))); } Ok(()) @@ -249,8 +240,9 @@ mod test { use super::*; use crate::tests::fixtures::docs::DocFixtures; + use crate::tests::fixtures::redirects::RedirectFixtures; use crate::tests::fixtures::wikihistory::WikihistoryFixtures; - use crate::utils::check_file_existence; + use crate::utils::test_utils::{check_file_existence, get_redirects_map}; use crate::wikihistory::test_get_wiki_history; #[test] @@ -381,6 +373,7 @@ mod test { ]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); let result = do_remove( "Web/API/ExampleOne", @@ -399,6 +392,14 @@ mod test { let wiki_history = test_get_wiki_history(Locale::EnUs); assert!(!wiki_history.contains_key("Web/API/ExampleOne")); assert!(wiki_history.contains_key("Web/API/RedirectTarget")); + + let redirects = get_redirects_map(Locale::EnUs); + assert_eq!(redirects.len(), 1); + assert!(redirects.contains_key("/en-US/docs/Web/API/ExampleOne")); + assert_eq!( + redirects.get("/en-US/docs/Web/API/ExampleOne").unwrap(), + "/en-US/docs/Web/API/RedirectTarget" + ); } #[test] @@ -410,6 +411,7 @@ mod test { ]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); let result = do_remove("Web/API/ExampleOne", Locale::EnUs, false, None, false); assert!(result.is_ok()); @@ -420,10 +422,12 @@ mod test { check_file_existence(root_path, &should_exist, &should_not_exist); let wiki_history = test_get_wiki_history(Locale::EnUs); - // println!("{:?}", wiki_history); assert!(!wiki_history.contains_key("Web/API/ExampleOne")); assert!(wiki_history.contains_key("Web/API/ExampleOne/Subpage")); assert!(wiki_history.contains_key("Web/API/RedirectTarget")); + + let redirects = get_redirects_map(Locale::EnUs); + assert_eq!(redirects.len(), 0); } #[test] @@ -434,6 +438,7 @@ mod test { ]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); let result = do_remove("Web/API/ExampleOne", Locale::EnUs, true, None, false); assert!(result.is_ok()); @@ -449,5 +454,100 @@ mod test { let wiki_history = test_get_wiki_history(Locale::EnUs); assert!(!wiki_history.contains_key("Web/API/ExampleOne")); assert!(!wiki_history.contains_key("Web/API/ExampleOne/Subpage")); + + let redirects = get_redirects_map(Locale::EnUs); + assert_eq!(redirects.len(), 0); + } + + #[test] + fn test_remove_recursive_with_redirect() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleOne/Subpage".to_string(), + "Web/API/RedirectTarget".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + + let result = do_remove( + "Web/API/ExampleOne", + Locale::EnUs, + true, + Some("Web/API/RedirectTarget"), + false, + ); + assert!(result.is_ok()); + + let should_exist = vec![]; + let should_not_exist = vec![ + "en-us/web/api/exampleone/index.md", + "en-us/web/api/exampleone/subpage/index.md", + ]; + let root_path = root_for_locale(Locale::EnUs).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::EnUs); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + assert!(!wiki_history.contains_key("Web/API/ExampleOne/Subpage")); + + let redirects = get_redirects_map(Locale::EnUs); + assert_eq!(redirects.len(), 2); + assert_eq!( + redirects.get("/en-US/docs/Web/API/ExampleOne").unwrap(), + "/en-US/docs/Web/API/RedirectTarget" + ); + assert_eq!( + redirects + .get("/en-US/docs/Web/API/ExampleOne/Subpage") + .unwrap(), + "/en-US/docs/Web/API/RedirectTarget" + ); + } + + #[test] + fn test_remove_recursive_with_redirect_translated() { + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/ExampleOne/Subpage".to_string(), + "Web/API/RedirectTarget".to_string(), + ]; + let _docs = DocFixtures::new(&slugs, Locale::PtBr); + let _wikihistory = WikihistoryFixtures::new(&slugs, Locale::PtBr); + let _redirects = RedirectFixtures::new(&vec![], Locale::PtBr); + + let result = do_remove( + "Web/API/ExampleOne", + Locale::PtBr, + true, + Some("Web/API/RedirectTarget"), + false, + ); + assert!(result.is_ok()); + + let should_exist = vec![]; + let should_not_exist = vec![ + "pt-br/web/api/exampleone/index.md", + "pt-br/web/api/exampleone/subpage/index.md", + ]; + let root_path = root_for_locale(Locale::PtBr).unwrap(); + check_file_existence(root_path, &should_exist, &should_not_exist); + + let wiki_history = test_get_wiki_history(Locale::PtBr); + assert!(!wiki_history.contains_key("Web/API/ExampleOne")); + assert!(!wiki_history.contains_key("Web/API/ExampleOne/Subpage")); + + let redirects = get_redirects_map(Locale::PtBr); + assert_eq!(redirects.len(), 2); + assert_eq!( + redirects.get("/pt-BR/docs/Web/API/ExampleOne").unwrap(), + "/pt-BR/docs/Web/API/RedirectTarget" + ); + assert_eq!( + redirects + .get("/pt-BR/docs/Web/API/ExampleOne/Subpage") + .unwrap(), + "/pt-BR/docs/Web/API/RedirectTarget" + ); } } diff --git a/crates/rari-tools/src/utils.rs b/crates/rari-tools/src/utils.rs index 38f85657..71814051 100644 --- a/crates/rari-tools/src/utils.rs +++ b/crates/rari-tools/src/utils.rs @@ -12,26 +12,48 @@ pub(crate) fn parent_slug(slug: &str) -> Result<&str, ToolError> { } #[cfg(test)] -use std::path::Path; -#[cfg(test)] -pub(crate) fn check_file_existence(root: &Path, should_exist: &[&str], should_not_exist: &[&str]) { - use std::path::PathBuf; - - for relative_path in should_exist { - let parts = relative_path.split('/').collect::>(); - let mut path = PathBuf::from(root); - for part in parts { - path.push(part); +pub mod test_utils { + use std::collections::HashMap; + use std::path::{Path, PathBuf}; + + use rari_doc::utils::root_for_locale; + use rari_types::locale::Locale; + + use crate::redirects; + pub(crate) fn check_file_existence( + root: &Path, + should_exist: &[&str], + should_not_exist: &[&str], + ) { + use std::path::PathBuf; + + for relative_path in should_exist { + let parts = relative_path.split('/').collect::>(); + let mut path = PathBuf::from(root); + for part in parts { + path.push(part); + } + assert!(path.exists(), "{} should exist", path.display()); } - assert!(path.exists(), "{} should exist", path.display()); - } - for relative_path in should_not_exist { - let parts = relative_path.split('/').collect::>(); - let mut path = PathBuf::from(root); - for part in parts { - path.push(part); + for relative_path in should_not_exist { + let parts = relative_path.split('/').collect::>(); + let mut path = PathBuf::from(root); + for part in parts { + path.push(part); + } + assert!(!path.exists(), "{} should not exist", path.display()); } - assert!(!path.exists(), "{} should not exist", path.display()); + } + + pub(crate) fn get_redirects_map(locale: Locale) -> HashMap { + let root_path = root_for_locale(locale).unwrap(); + + let mut redirects_path = PathBuf::from(root_path); + redirects_path.push(locale.as_folder_str()); + redirects_path.push("_redirects.txt"); + let mut redirects = HashMap::new(); + redirects::read_redirects_raw(&redirects_path, &mut redirects).unwrap(); + redirects } } From 8e22baca75bbb3f5e5d67f108e5e35f12da5919e Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 10 Oct 2024 18:12:32 +0200 Subject: [PATCH 07/16] added dangling references check to delete --- Cargo.lock | 1 + crates/rari-tools/Cargo.toml | 1 + crates/rari-tools/src/remove.rs | 50 ++++++++++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 502fa066..1d8dee3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2705,6 +2705,7 @@ dependencies = [ "rari-doc", "rari-types", "rari-utils", + "rayon", "reqwest", "serde", "serde_json", diff --git a/crates/rari-tools/Cargo.toml b/crates/rari-tools/Cargo.toml index eff01521..24d8bd2b 100644 --- a/crates/rari-tools/Cargo.toml +++ b/crates/rari-tools/Cargo.toml @@ -18,6 +18,7 @@ tracing.workspace = true reqwest.workspace = true url.workspace = true indoc.workspace = true +rayon.workspace = true console = "0" dialoguer = "0" diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 3ae90b4d..5b70cb97 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::BTreeSet; use std::ffi::OsStr; use std::path::PathBuf; use std::process::Command; @@ -7,11 +8,15 @@ use std::str::FromStr; use console::Style; use dialoguer::theme::ColorfulTheme; use dialoguer::Confirm; +use rari_doc::error::DocError; use rari_doc::helpers::subpages::get_sub_pages; use rari_doc::pages::page::{self, PageCategory, PageLike}; +use rari_doc::pages::types::doc::Doc; +use rari_doc::reader::read_docs_parallel; use rari_doc::resolve::{build_url, url_meta_from, UrlMeta}; use rari_doc::utils::root_for_locale; use rari_types::locale::Locale; +use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::error::ToolError; use crate::redirects::add_redirects; @@ -73,15 +78,55 @@ pub fn remove( .unwrap_or_default() { let removed = do_remove(slug, locale, recursive, redirect, false)?; + let removed_urls = removed + .iter() + .map(|slug| build_url(slug, locale, PageCategory::Doc)) + .collect::, DocError>>()?; println!( "{} {} {}", green.apply_to("Deleted"), bold.apply_to(removed.len()), - green.apply_to("documents"), + green.apply_to("documents:"), ); + for url in &removed_urls { + println!("{}", red.apply_to(&url)); + } // Find references to deleted documents and // list them for manual review + println!("Checking references to deleted documents..."); + let mut docs_path = PathBuf::from(root_for_locale(locale)?); + docs_path.push(locale.as_folder_str()); + + let docs = read_docs_parallel::(&[docs_path], None)?; + + let referencing_docs: BTreeSet = docs + .into_par_iter() + .filter_map(|doc| { + for url in &removed { + if doc.content().contains(url) { + return Some(doc.url().to_owned()); + } + } + None + }) + .collect(); + + if referencing_docs.is_empty() { + println!( + "{}", + green.apply_to("No file is referring to the deleted document."), + ); + } else { + println!( + "{} {}", + yellow.apply_to(referencing_docs.len()), + yellow.apply_to("files are referring to the deleted documents. Please update the following files to remove the links:"), + ); + for url in &referencing_docs { + println!("{}", yellow.apply_to(url)); + } + } } Ok(()) } @@ -207,9 +252,6 @@ fn do_remove( .collect::, ToolError>>()?; add_redirects(locale, &pairs)?; } - - // check references to the removed documents (do it in the main method) - Ok(slugs_to_remove) } From 3626947360de516fa98c691c1be8c33b3dcfacfe Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 10 Oct 2024 18:18:46 +0200 Subject: [PATCH 08/16] cleanup --- crates/rari-cli/main.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/crates/rari-cli/main.rs b/crates/rari-cli/main.rs index b729e534..a87ef4eb 100644 --- a/crates/rari-cli/main.rs +++ b/crates/rari-cli/main.rs @@ -349,14 +349,6 @@ fn main() -> Result<(), Error> { )?; } ContentSubcommand::Delete(args) => { - // slug: String, - // locale: Option, - // #[arg(short, long, default_value_t = false)] - // recursive: bool, - // #[arg(long)] - // redirect: Option, - // #[arg(short = 'y', long, help = "Assume yes to all prompts")] - // assume_yes: bool, remove( &args.slug, args.locale.as_deref(), From 6af1f34cf07ca176b95f4d70db48270e0830b54e Mon Sep 17 00:00:00 2001 From: Florian Dieminger Date: Fri, 11 Oct 2024 11:33:47 +0200 Subject: [PATCH 09/16] move git code and nits --- crates/rari-tools/src/git.rs | 43 +++++++++++++++++++++++++ crates/rari-tools/src/history.rs | 13 ++++---- crates/rari-tools/src/lib.rs | 1 + crates/rari-tools/src/move.rs | 24 ++++---------- crates/rari-tools/src/remove.rs | 55 ++++++++++++-------------------- 5 files changed, 79 insertions(+), 57 deletions(-) create mode 100644 crates/rari-tools/src/git.rs diff --git a/crates/rari-tools/src/git.rs b/crates/rari-tools/src/git.rs new file mode 100644 index 00000000..dc62fac2 --- /dev/null +++ b/crates/rari-tools/src/git.rs @@ -0,0 +1,43 @@ +use std::ffi::OsStr; +use std::path::Path; +use std::process::{Command, Output}; + +/// Run `git` with `args` wiht `root` as current directory. +/// For tests this will run the first arg as command, eg.: +/// Instead of `git mv foo bar` -> `mv foo bar`. +pub fn exec_git_with_test_fallback(args: &[impl AsRef], root: impl AsRef) -> Output { + let git = OsStr::new("git"); + let echo = OsStr::new("echo"); + + let (command, args) = if cfg!(test) { + ( + args.first().map(AsRef::as_ref).unwrap_or(echo), + &args[if args.is_empty() { 0 } else { 1 }..], + ) + } else { + (git, args) + }; + exec_git_internal(command, args, root) +} + +pub fn exec_git(args: &[impl AsRef], root: impl AsRef) -> Output { + let output = Command::new("git") + .args(args) + .current_dir(root) + .output() + .expect("failed to execute process"); + output +} + +fn exec_git_internal( + command: impl AsRef, + args: &[impl AsRef], + root: impl AsRef, +) -> Output { + let output = Command::new(command) + .args(args) + .current_dir(root) + .output() + .expect("failed to execute process"); + output +} diff --git a/crates/rari-tools/src/history.rs b/crates/rari-tools/src/history.rs index 8852e6f3..f9be758f 100644 --- a/crates/rari-tools/src/history.rs +++ b/crates/rari-tools/src/history.rs @@ -7,6 +7,8 @@ use std::process::Command; use rari_types::globals::content_root; use rari_types::HistoryEntry; +use crate::git::exec_git; + pub fn gather_history() -> BTreeMap { modification_times() } @@ -23,8 +25,8 @@ fn modification_times(//path: &Path, let repo_root_raw = String::from_utf8_lossy(&output.stdout); let repo_root = repo_root_raw.trim(); - let output = Command::new("git") - .args([ + exec_git( + &[ "log", "--name-only", "--no-decorate", @@ -32,10 +34,9 @@ fn modification_times(//path: &Path, "--date-order", "--reverse", "-z", - ]) - .current_dir(repo_root) - .output() - .expect("failed to execute process"); + ], + repo_root, + ); let output_str = String::from_utf8_lossy(&output.stdout); let mut history = BTreeMap::new(); diff --git a/crates/rari-tools/src/lib.rs b/crates/rari-tools/src/lib.rs index d1045eea..41143aed 100644 --- a/crates/rari-tools/src/lib.rs +++ b/crates/rari-tools/src/lib.rs @@ -1,4 +1,5 @@ pub mod error; +pub mod git; pub mod history; pub mod r#move; pub mod popularities; diff --git a/crates/rari-tools/src/move.rs b/crates/rari-tools/src/move.rs index a9cf18ee..222e9d6c 100644 --- a/crates/rari-tools/src/move.rs +++ b/crates/rari-tools/src/move.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use std::ffi::OsStr; use std::fs::create_dir_all; use std::path::PathBuf; -use std::process::Command; use std::str::FromStr; use std::sync::Arc; @@ -18,6 +17,7 @@ use rari_doc::{ use rari_types::locale::Locale; use crate::error::ToolError; +use crate::git::exec_git_with_test_fallback; use crate::redirects::add_redirects; use crate::utils::parent_slug; use crate::wikihistory::update_wiki_history; @@ -170,25 +170,15 @@ fn do_move( )); } - // Conditional command for testing. In testing, we do not use git, because the test - // fixtures are not under git control. Instead of `git mv …` we use `mv …`. - let command = if cfg!(test) { "mv" } else { "git" }; - let args = if cfg!(test) { - vec![old_folder_path.as_os_str(), new_folder_path.as_os_str()] - } else { - vec![ + // Execute the git move. + let output = exec_git_with_test_fallback( + &[ OsStr::new("mv"), old_folder_path.as_os_str(), new_folder_path.as_os_str(), - ] - }; - - // Execute the git move. - let output = Command::new(command) - .args(args) - .current_dir(root_for_locale(locale)?) - .output() - .expect("failed to execute process"); + ], + root_for_locale(locale)?, + ); if !output.status.success() { return Err(ToolError::GitError(format!( diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 5b70cb97..5db72fd8 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::ffi::OsStr; use std::path::PathBuf; -use std::process::Command; use std::str::FromStr; use console::Style; @@ -13,12 +12,13 @@ use rari_doc::helpers::subpages::get_sub_pages; use rari_doc::pages::page::{self, PageCategory, PageLike}; use rari_doc::pages::types::doc::Doc; use rari_doc::reader::read_docs_parallel; -use rari_doc::resolve::{build_url, url_meta_from, UrlMeta}; +use rari_doc::resolve::build_url; use rari_doc::utils::root_for_locale; use rari_types::locale::Locale; -use rayon::iter::{IntoParallelIterator, ParallelIterator}; +use rayon::iter::{once, IntoParallelIterator, ParallelIterator}; use crate::error::ToolError; +use crate::git::exec_git_with_test_fallback; use crate::redirects::add_redirects; use crate::wikihistory::delete_from_wiki_history; @@ -169,13 +169,12 @@ fn do_remove( } let slugs_to_remove = if recursive { - [doc.clone()] - .iter() + once(&doc) .chain(&subpages) - .map(|page_ref| page_ref.slug().to_owned()) + .map(|page_ref| page_ref.slug().to_string()) .collect::>() } else { - vec![real_slug.to_owned()] + vec![real_slug.to_string()] }; if dry_run { @@ -188,25 +187,21 @@ fn do_remove( // Conditional command for testing. In testing, we do not use git, because the test // fixtures are not under git control. Instead of `git rm …` we use `rm …`. - let mut path = PathBuf::from(locale.as_folder_str()); - let url = build_url(real_slug, locale, PageCategory::Doc)?; - let UrlMeta { folder_path, .. } = url_meta_from(&url)?; - path.push(folder_path); + let path = doc.path(); - let command = if cfg!(test) { "rm" } else { "git" }; if recursive { - let args = if cfg!(test) { - vec![OsStr::new("-rf"), path.as_os_str()] - } else { - vec![OsStr::new("rm"), OsStr::new("-rf"), path.as_os_str()] - }; + let parent = path + .parent() + .ok_or(ToolError::InvalidSlug(Cow::Owned(format!( + "{slug} ({}) has no parent directory", + path.display() + ))))?; // Execute the recursive remove command - let output = Command::new(command) - .args(args) - .current_dir(root_for_locale(locale)?) - .output() - .expect("failed to execute process"); + let output = exec_git_with_test_fallback( + &[OsStr::new("rm"), OsStr::new("-rf"), parent.as_os_str()], + root_for_locale(locale)?, + ); if !output.status.success() { return Err(ToolError::GitError(format!( @@ -215,19 +210,11 @@ fn do_remove( ))); } } else { - path.push("index.md"); - let args = if cfg!(test) { - vec![path.as_os_str()] - } else { - vec![OsStr::new("rm"), &path.as_os_str()] - }; - // Execute the single file remove command - let output = Command::new(command) - .args(args) - .current_dir(root_for_locale(locale)?) - .output() - .expect("failed to execute process"); + let output = exec_git_with_test_fallback( + &[OsStr::new("rm"), path.as_os_str()], + root_for_locale(locale)?, + ); if !output.status.success() { return Err(ToolError::GitError(format!( From 4a2038e2adaebca0fa8da8a8909afff6d62ae4f6 Mon Sep 17 00:00:00 2001 From: Florian Dieminger Date: Fri, 11 Oct 2024 12:17:46 +0200 Subject: [PATCH 10/16] fix history --- crates/rari-tools/src/history.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rari-tools/src/history.rs b/crates/rari-tools/src/history.rs index f9be758f..024b4014 100644 --- a/crates/rari-tools/src/history.rs +++ b/crates/rari-tools/src/history.rs @@ -25,7 +25,7 @@ fn modification_times(//path: &Path, let repo_root_raw = String::from_utf8_lossy(&output.stdout); let repo_root = repo_root_raw.trim(); - exec_git( + let output = exec_git( &[ "log", "--name-only", From ea4a4ca2339cf75c59edd79b46dfe35fd1ea39a4 Mon Sep 17 00:00:00 2001 From: Florian Dieminger Date: Fri, 11 Oct 2024 13:53:53 +0200 Subject: [PATCH 11/16] nit --- crates/rari-tools/src/remove.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/rari-tools/src/remove.rs b/crates/rari-tools/src/remove.rs index 5db72fd8..99306f9e 100644 --- a/crates/rari-tools/src/remove.rs +++ b/crates/rari-tools/src/remove.rs @@ -138,8 +138,7 @@ fn do_remove( redirect: Option<&str>, dry_run: bool, ) -> Result, ToolError> { - let url = build_url(slug, locale, PageCategory::Doc)?; - let doc = page::Page::from_url(&url)?; + let doc = Doc::page_from_slug(slug, locale)?; let real_slug = doc.slug(); // If we get a redirect value passed in, it is either a slug or a complete url. @@ -160,7 +159,7 @@ fn do_remove( None }; - let subpages = get_sub_pages(&url, None, Default::default())?; + let subpages = get_sub_pages(doc.url(), None, Default::default())?; if !recursive && !subpages.is_empty() && redirect.is_some() { return Err(ToolError::HasSubpagesError(Cow::Owned(format!( "{0}, unable to remove and redirect a document with children", From 30f393c1c0ef7d11531e48f947d8b89b1aa18465 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Fri, 11 Oct 2024 20:16:27 +0200 Subject: [PATCH 12/16] added add-redirect command --- crates/rari-cli/main.rs | 10 +++ crates/rari-tools/src/add_redirect.rs | 102 ++++++++++++++++++++++++++ crates/rari-tools/src/error.rs | 2 + crates/rari-tools/src/lib.rs | 1 + 4 files changed, 115 insertions(+) create mode 100644 crates/rari-tools/src/add_redirect.rs diff --git a/crates/rari-cli/main.rs b/crates/rari-cli/main.rs index a87ef4eb..3c0c5b8e 100644 --- a/crates/rari-cli/main.rs +++ b/crates/rari-cli/main.rs @@ -66,6 +66,7 @@ enum ContentSubcommand { /// Moves content from one slug to another Move(MoveArgs), Delete(DeleteArgs), + AddRedirect(AddRedirectArgs), } #[derive(Args)] @@ -89,6 +90,12 @@ struct DeleteArgs { assume_yes: bool, } +#[derive(Args)] +struct AddRedirectArgs { + from_url: String, + to_url: String, +} + #[derive(Args)] struct UpdateArgs { #[arg(long)] @@ -357,6 +364,9 @@ fn main() -> Result<(), Error> { args.assume_yes, )?; } + ContentSubcommand::AddRedirect(_args) => { + todo!() + } }, Commands::Update(args) => update(args.version)?, } diff --git a/crates/rari-tools/src/add_redirect.rs b/crates/rari-tools/src/add_redirect.rs new file mode 100644 index 00000000..7c393d5c --- /dev/null +++ b/crates/rari-tools/src/add_redirect.rs @@ -0,0 +1,102 @@ +use std::borrow::Cow; + +use rari_doc::pages::page; +use rari_doc::resolve::{url_meta_from, UrlMeta}; + +use crate::error::ToolError; +use crate::redirects::add_redirects; + +pub fn add_redirect(from_url: &str, to_url: &str) -> Result<(), ToolError> { + do_add_redirect(from_url, to_url) + // console output +} + +fn do_add_redirect(from_url: &str, to_url: &str) -> Result<(), ToolError> { + validate_args(from_url, to_url)?; + let UrlMeta { + locale: from_locale, + .. + } = url_meta_from(from_url)?; + if !to_url.starts_with("http") { + if from_url == to_url { + return Err(ToolError::InvalidRedirectToURL(format!( + "redirect url is the same as the from url: {to_url}" + ))); + } + + if !page::Page::exists(to_url) { + return Err(ToolError::InvalidRedirectToURL(format!( + "redirect url does not exist: {to_url}" + ))); + } + let UrlMeta { + locale: to_locale, .. + } = url_meta_from(to_url)?; + if from_locale != to_locale { + return Err(ToolError::InvalidRedirectToURL(format!( + "redirect url locales do not match: {from_locale} != {to_locale}" + ))); + } + } + add_redirects(from_locale, &[(from_url.to_owned(), to_url.to_owned())])?; + Ok(()) +} + +fn validate_args(from_url: &str, to_url: &str) -> Result<(), ToolError> { + if from_url.is_empty() { + return Err(ToolError::InvalidUrl(Cow::Borrowed( + "from_url cannot be empty", + ))); + } + if to_url.is_empty() { + return Err(ToolError::InvalidUrl(Cow::Borrowed( + "to_url cannot be empty", + ))); + } + if from_url.contains("#") { + return Err(ToolError::InvalidUrl(Cow::Borrowed( + "from_url cannot contain '#'", + ))); + } + Ok(()) +} + +// These tests use file system fixtures to simulate content and translated content. +// The file system is a shared resource, so we force tests to be run serially, +// to avoid concurrent fixture management issues. +// Using `file_serial` as a synchonization lock, we run all tests using +// the same `key` (here: file_fixtures) to be serialized across modules. +#[cfg(test)] +use serial_test::file_serial; +#[cfg(test)] +#[file_serial(file_fixtures)] +mod test { + use rari_types::locale::Locale; + + use super::*; + use crate::tests::fixtures::docs::DocFixtures; + use crate::tests::fixtures::redirects::RedirectFixtures; + use crate::utils::test_utils::get_redirects_map; + + #[test] + fn test_add_redirect() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + + let result = do_add_redirect( + "/en-US/docs/Web/API/ExampleGone", + "/en-US/docs/Web/API/ExampleOne", + ); + println!("{:?}", result); + assert!(result.is_ok()); + + let redirects = get_redirects_map(Locale::EnUs); + assert_eq!(redirects.len(), 1); + assert!(redirects.contains_key("/en-US/docs/Web/API/ExampleGone")); + assert_eq!( + redirects.get("/en-US/docs/Web/API/ExampleGone").unwrap(), + "/en-US/docs/Web/API/ExampleOne" + ); + } +} diff --git a/crates/rari-tools/src/error.rs b/crates/rari-tools/src/error.rs index 23d99f75..66e0125d 100644 --- a/crates/rari-tools/src/error.rs +++ b/crates/rari-tools/src/error.rs @@ -10,6 +10,8 @@ use thiserror::Error; pub enum ToolError { #[error("Invalid slug: {0}")] InvalidSlug(Cow<'static, str>), + #[error("Invalid url: {0}")] + InvalidUrl(Cow<'static, str>), #[error("Git error: {0}")] GitError(String), diff --git a/crates/rari-tools/src/lib.rs b/crates/rari-tools/src/lib.rs index 41143aed..68d182da 100644 --- a/crates/rari-tools/src/lib.rs +++ b/crates/rari-tools/src/lib.rs @@ -1,3 +1,4 @@ +pub mod add_redirect; pub mod error; pub mod git; pub mod history; From 1a1425b3d69d764ebdfe2a38dfb7c7aacaff4fa4 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Sat, 12 Oct 2024 18:09:04 +0200 Subject: [PATCH 13/16] depend on redirects module for some validation, fixes, tests --- crates/rari-tools/src/add_redirect.rs | 70 +++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/crates/rari-tools/src/add_redirect.rs b/crates/rari-tools/src/add_redirect.rs index 7c393d5c..1df77701 100644 --- a/crates/rari-tools/src/add_redirect.rs +++ b/crates/rari-tools/src/add_redirect.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; -use rari_doc::pages::page; use rari_doc::resolve::{url_meta_from, UrlMeta}; +use rari_types::locale::Locale; use crate::error::ToolError; use crate::redirects::add_redirects; @@ -24,15 +24,12 @@ fn do_add_redirect(from_url: &str, to_url: &str) -> Result<(), ToolError> { ))); } - if !page::Page::exists(to_url) { - return Err(ToolError::InvalidRedirectToURL(format!( - "redirect url does not exist: {to_url}" - ))); - } let UrlMeta { locale: to_locale, .. } = url_meta_from(to_url)?; - if from_locale != to_locale { + + // Enforce that the locales match or the target is in the default Locale + if from_locale != to_locale && to_locale != Locale::EnUs { return Err(ToolError::InvalidRedirectToURL(format!( "redirect url locales do not match: {from_locale} != {to_locale}" ))); @@ -82,21 +79,74 @@ mod test { fn test_add_redirect() { let slugs = vec!["Web/API/ExampleOne".to_string()]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); - let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + let _redirects = RedirectFixtures::new( + &vec![( + "docs/Web/API/Something".to_string(), + "docs/Web/API/SomethingElse".to_string(), + )], + Locale::EnUs, + ); let result = do_add_redirect( "/en-US/docs/Web/API/ExampleGone", "/en-US/docs/Web/API/ExampleOne", ); - println!("{:?}", result); assert!(result.is_ok()); let redirects = get_redirects_map(Locale::EnUs); - assert_eq!(redirects.len(), 1); + assert_eq!(redirects.len(), 2); assert!(redirects.contains_key("/en-US/docs/Web/API/ExampleGone")); assert_eq!( redirects.get("/en-US/docs/Web/API/ExampleGone").unwrap(), "/en-US/docs/Web/API/ExampleOne" ); } + + #[test] + fn test_add_redirect_differing_locales() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _docs_pt = DocFixtures::new(&slugs, Locale::PtBr); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + let _redirects_pt = RedirectFixtures::new(&vec![], Locale::PtBr); + + // Locales do not match + let result = do_add_redirect( + "/en-US/docs/Web/API/ExampleGone", + "/pt-BR/docs/Web/API/ExampleOne", + ); + assert!(result.is_err()); + assert!(matches!(result, Err(ToolError::InvalidRedirectToURL(_)))); + + // Target is en-US, even if locales differ + let result = do_add_redirect( + "/pt-BR/docs/Web/API/ExampleGone", + "/en-US/docs/Web/API/ExampleOne", + ); + assert!(result.is_ok()); + + let redirects = get_redirects_map(Locale::PtBr); + assert!(redirects.contains_key("/pt-BR/docs/Web/API/ExampleGone")); + assert_eq!( + redirects.get("/pt-BR/docs/Web/API/ExampleGone").unwrap(), + "/en-US/docs/Web/API/ExampleOne" + ); + } + + #[test] + fn test_add_redirect_external() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + + let result = do_add_redirect("/en-US/docs/Web/API/ExampleGone", "https://example.com/"); + assert!(result.is_ok()); + + let redirects = get_redirects_map(Locale::EnUs); + assert!(redirects.contains_key("/en-US/docs/Web/API/ExampleGone")); + assert_eq!( + redirects.get("/en-US/docs/Web/API/ExampleGone").unwrap(), + "https://example.com/" + ); + } } From cb9ed6dfaf456ec58d1710cdc6eacbf87818408a Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Mon, 14 Oct 2024 11:20:28 +0200 Subject: [PATCH 14/16] added console output, cli integration --- crates/rari-cli/main.rs | 5 +++-- crates/rari-tools/src/add_redirect.rs | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/rari-cli/main.rs b/crates/rari-cli/main.rs index 3c0c5b8e..d3d37c37 100644 --- a/crates/rari-cli/main.rs +++ b/crates/rari-cli/main.rs @@ -18,6 +18,7 @@ use rari_doc::pages::types::doc::Doc; use rari_doc::reader::read_docs_parallel; use rari_doc::search_index::build_search_index; use rari_doc::utils::TEMPL_RECORDER_SENDER; +use rari_tools::add_redirect::add_redirect; use rari_tools::history::gather_history; use rari_tools::popularities::update_popularities; use rari_tools::r#move::r#move; @@ -364,8 +365,8 @@ fn main() -> Result<(), Error> { args.assume_yes, )?; } - ContentSubcommand::AddRedirect(_args) => { - todo!() + ContentSubcommand::AddRedirect(args) => { + add_redirect(&args.from_url, &args.to_url)?; } }, Commands::Update(args) => update(args.version)?, diff --git a/crates/rari-tools/src/add_redirect.rs b/crates/rari-tools/src/add_redirect.rs index 1df77701..527fef45 100644 --- a/crates/rari-tools/src/add_redirect.rs +++ b/crates/rari-tools/src/add_redirect.rs @@ -1,5 +1,6 @@ use std::borrow::Cow; +use console::Style; use rari_doc::resolve::{url_meta_from, UrlMeta}; use rari_types::locale::Locale; @@ -7,8 +8,20 @@ use crate::error::ToolError; use crate::redirects::add_redirects; pub fn add_redirect(from_url: &str, to_url: &str) -> Result<(), ToolError> { - do_add_redirect(from_url, to_url) - // console output + do_add_redirect(from_url, to_url)?; + + let green = Style::new().green(); + let bold = Style::new().bold(); + + println!( + "{} {} {} {}", + green.apply_to("Saved"), + bold.apply_to(from_url), + green.apply_to("→"), + bold.apply_to(to_url), + ); + + Ok(()) } fn do_add_redirect(from_url: &str, to_url: &str) -> Result<(), ToolError> { From 00d4ae2b16b8a2bc816bdbe110fdc7f86c13794f Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Mon, 14 Oct 2024 14:12:55 +0200 Subject: [PATCH 15/16] fixed a bug in redirect logic, added some tests, make locale always pass by value in a few cases --- crates/rari-tools/src/add_redirect.rs | 18 +++- crates/rari-tools/src/move.rs | 5 +- crates/rari-tools/src/redirects.rs | 143 +++++++++++++++++--------- 3 files changed, 116 insertions(+), 50 deletions(-) diff --git a/crates/rari-tools/src/add_redirect.rs b/crates/rari-tools/src/add_redirect.rs index 527fef45..c21c074a 100644 --- a/crates/rari-tools/src/add_redirect.rs +++ b/crates/rari-tools/src/add_redirect.rs @@ -90,7 +90,10 @@ mod test { #[test] fn test_add_redirect() { - let slugs = vec!["Web/API/ExampleOne".to_string()]; + let slugs = vec![ + "Web/API/ExampleOne".to_string(), + "Web/API/SomethingElse".to_string(), + ]; let _docs = DocFixtures::new(&slugs, Locale::EnUs); let _redirects = RedirectFixtures::new( &vec![( @@ -115,6 +118,19 @@ mod test { ); } + #[test] + fn test_add_redirect_missing_target() { + let slugs = vec!["Web/API/ExampleOne".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let _redirects = RedirectFixtures::new(&vec![], Locale::EnUs); + + let result = do_add_redirect( + "/en-US/docs/Web/API/ExampleGone", + "/en-US/docs/Web/API/ExampleMissing", + ); + assert!(result.is_err()); + } + #[test] fn test_add_redirect_differing_locales() { let slugs = vec!["Web/API/ExampleOne".to_string()]; diff --git a/crates/rari-tools/src/move.rs b/crates/rari-tools/src/move.rs index 222e9d6c..d1c66a6b 100644 --- a/crates/rari-tools/src/move.rs +++ b/crates/rari-tools/src/move.rs @@ -318,6 +318,7 @@ mod test { "Web/API/ExampleOne".to_string(), "Web/API/ExampleOne/SubExampleOne".to_string(), "Web/API/ExampleOne/SubExampleTwo".to_string(), + "Web/API/SomethingElse".to_string(), ]; let redirects = vec![ ( @@ -353,7 +354,7 @@ mod test { Locale::EnUs, false, ); - + println!("result: {:?}", result); assert!(result.is_ok()); let result = result.unwrap(); assert!(result.len() == 3); @@ -428,6 +429,7 @@ mod test { "Web/API/ExampleOne".to_string(), "Web/API/ExampleOne/SubExampleOne".to_string(), "Web/API/ExampleOne/SubExampleTwo".to_string(), + "Web/API/SomethingElse".to_string(), ]; let redirects = vec![ ( @@ -463,7 +465,6 @@ mod test { Locale::PtBr, false, ); - assert!(result.is_ok()); let result = result.unwrap(); assert!(result.len() == 3); diff --git a/crates/rari-tools/src/redirects.rs b/crates/rari-tools/src/redirects.rs index 91bab089..3e05606d 100644 --- a/crates/rari-tools/src/redirects.rs +++ b/crates/rari-tools/src/redirects.rs @@ -237,7 +237,7 @@ pub fn add_redirects(locale: Locale, update_pairs: &[(String, String)]) -> Resul let clean_pairs: HashMap = short_cuts(&clean_pairs)?.into_iter().collect(); - validate_pairs(&clean_pairs, &locale)?; + validate_pairs(&clean_pairs, locale)?; // Write the updated map back to the redirects file write_redirects(&path, &clean_pairs)?; @@ -258,7 +258,7 @@ pub fn add_redirects(locale: Locale, update_pairs: &[(String, String)]) -> Resul /// /// * `Ok(())` if all pairs are valid. /// * `Err(ToolError)` if any pair is invalid. -fn validate_pairs(pairs: &HashMap, locale: &Locale) -> Result<(), ToolError> { +fn validate_pairs(pairs: &HashMap, locale: Locale) -> Result<(), ToolError> { for (from, to) in pairs { validate_from_url(from, locale)?; validate_to_url(to, locale)?; @@ -284,7 +284,7 @@ fn validate_pairs(pairs: &HashMap, locale: &Locale) -> Result<() /// /// * `Ok(())` if the URL is valid. /// * `Err(ToolError)` if the URL is invalid. -fn validate_from_url(url: &str, locale: &Locale) -> Result<(), ToolError> { +fn validate_from_url(url: &str, locale: Locale) -> Result<(), ToolError> { let url = url.to_lowercase(); if !url.starts_with('/') { return Err(ToolError::InvalidRedirectFromURL(format!( @@ -347,7 +347,7 @@ fn validate_from_url(url: &str, locale: &Locale) -> Result<(), ToolError> { /// /// * `Ok(())` if the URL is valid. /// * `Err(ToolError)` if the URL is invalid. -fn validate_to_url(url: &str, locale: &Locale) -> Result<(), ToolError> { +fn validate_to_url(url: &str, locale: Locale) -> Result<(), ToolError> { if is_vanity_redirect_url(url) { return Ok(()); } @@ -364,31 +364,25 @@ fn validate_to_url(url: &str, locale: &Locale) -> Result<(), ToolError> { } } else if url.starts_with('/') { // Internal URL, perform validations - check_url_invalid_symbols(url)?; - validate_url_locale(url)?; // Split by '#', take the bare URL let bare_url = url.split('#').next().unwrap_or(""); - let parts: Vec<&str> = bare_url.split('/').collect(); - if parts.len() < 2 { - return Err(ToolError::InvalidRedirectToURL(format!( - "To-URL '{}' does not have enough parts for locale validation.", - url - ))); - } - - let to_locale = parts[1]; - if to_locale != locale.as_url_str().to_lowercase() { - // Different locale, no need to check path + let UrlMeta { + folder_path: path, + locale: to_locale, + .. + } = url_meta_from(bare_url)?; + + // TODO: Revisit this logic, why is a different locale (usecase: redirecting from + // translated content into en-US) a free pass on checking if the target document exists? + if to_locale != locale { + // Different locale, no need to check path? return Ok(()); } - let UrlMeta { - folder_path: path, .. - } = url_meta_from(url)?; - let path = root_for_locale(*locale)? + let path = root_for_locale(locale)? .join(locale.as_folder_str()) .join(path); if !path.exists() { @@ -408,25 +402,25 @@ fn validate_to_url(url: &str, locale: &Locale) -> Result<(), ToolError> { Ok(()) } -fn validate_url_locale(url: &str) -> Result<(), ToolError> { - let parts: Vec<&str> = url.split('/').collect(); - if parts.len() < 3 { - return Err(ToolError::InvalidRedirectToURL(format!( - "To-URL '{}' does not have enough parts for locale validation.", - url - ))); - } - - let to_locale = parts[1]; - if Locale::from_str(to_locale).is_err() { - return Err(ToolError::InvalidRedirectToURL(format!( - "Locale prefix '{}' in To-URL '{}' is not valid.", - to_locale, url - ))); - } - - Ok(()) -} +// fn validate_url_locale(url: &str) -> Result<(), ToolError> { +// let parts: Vec<&str> = url.split('/').collect(); +// if parts.len() < 3 { +// return Err(ToolError::InvalidRedirectToURL(format!( +// "To-URL '{}' does not have enough parts for locale validation.", +// url +// ))); +// } + +// let to_locale = parts[1]; +// if Locale::from_str(to_locale).is_err() { +// return Err(ToolError::InvalidRedirectToURL(format!( +// "Locale prefix '{}' in To-URL '{}' is not valid.", +// to_locale, url +// ))); +// } + +// Ok(()) +// } fn is_vanity_redirect_url(url: &str) -> bool { url.strip_prefix('/') @@ -649,8 +643,12 @@ where } #[cfg(test)] +use serial_test::file_serial; +#[cfg(test)] +#[file_serial(file_fixtures)] mod tests { use super::*; + use crate::tests::fixtures::docs::DocFixtures; fn s(s: &str) -> String { s.to_string() @@ -849,11 +847,62 @@ mod tests { ); } + #[test] + fn test_validate_to_url_happy_path() { + let slugs = vec!["A".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let url = "/en-US/docs/A"; + let locale = Locale::EnUs; + let result = validate_to_url(url, locale); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_to_url_non_existing_doc() { + let slugs = vec!["A".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let url = "/en-US/docs/B"; + let locale = Locale::EnUs; + let result = validate_to_url(url, locale); + assert!(result.is_err()); + } + + #[test] + fn test_validate_to_url_no_enough_parts() { + let slugs = vec!["A".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); + let url = "/"; + let locale = Locale::EnUs; + let result = validate_to_url(url, locale); + assert!(result.is_err()); + + let url = "/en-US/A"; + let locale = Locale::EnUs; + let result = validate_to_url(url, locale); + assert!(result.is_err()); + } + + #[test] + fn test_validate_to_url_invalid_symbols() { + let url = "/en-US/docs/\nA"; + let locale = Locale::EnUs; + let result = validate_to_url(url, locale); + assert!(result.is_err()); + } + + #[test] + fn test_validate_to_url_diff_locale() { + let url = "/en-US/docs/A"; + let locale = Locale::PtBr; + let result = validate_to_url(url, locale); + assert!(result.is_ok()); + } + #[test] fn test_validate_from_url_happy_path() { let url = "/en-US/docs/A"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_ok()); } @@ -861,7 +910,7 @@ mod tests { fn test_validate_from_url_url_does_not_start_with_slash() { let url = "en-US/docs/A"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } @@ -869,7 +918,7 @@ mod tests { fn test_validate_from_url_url_has_insufficient_parts() { let url = "/en-US/docs"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } @@ -877,7 +926,7 @@ mod tests { fn test_validate_from_url_locale_mismatch() { let url = "/pt-BR/docs/A"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } @@ -885,7 +934,7 @@ mod tests { fn test_validate_from_url_missing_docs_segment() { let url = "/en-US/A"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } @@ -893,7 +942,7 @@ mod tests { fn test_validate_from_url_invalid_locale_prefix() { let url = "/hu/docs/A"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } @@ -901,7 +950,7 @@ mod tests { fn test_validate_from_url_forbidden_symbol() { let url = "/en-US/docs/\nA"; let locale = Locale::EnUs; - let result = validate_from_url(url, &locale); + let result = validate_from_url(url, locale); assert!(result.is_err()); } From 05a5d81fa657e820ddafc38ef39c8f2406c1e4cb Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Tue, 15 Oct 2024 10:23:00 +0200 Subject: [PATCH 16/16] removed early return for differing locales on target url validation, removed commented code --- crates/rari-tools/src/redirects.rs | 34 +++++------------------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/crates/rari-tools/src/redirects.rs b/crates/rari-tools/src/redirects.rs index 3e05606d..dcdbf995 100644 --- a/crates/rari-tools/src/redirects.rs +++ b/crates/rari-tools/src/redirects.rs @@ -375,15 +375,8 @@ fn validate_to_url(url: &str, locale: Locale) -> Result<(), ToolError> { .. } = url_meta_from(bare_url)?; - // TODO: Revisit this logic, why is a different locale (usecase: redirecting from - // translated content into en-US) a free pass on checking if the target document exists? - if to_locale != locale { - // Different locale, no need to check path? - return Ok(()); - } - - let path = root_for_locale(locale)? - .join(locale.as_folder_str()) + let path = root_for_locale(to_locale)? + .join(to_locale.as_folder_str()) .join(path); if !path.exists() { return Err(ToolError::InvalidRedirectToURL(format!( @@ -402,26 +395,6 @@ fn validate_to_url(url: &str, locale: Locale) -> Result<(), ToolError> { Ok(()) } -// fn validate_url_locale(url: &str) -> Result<(), ToolError> { -// let parts: Vec<&str> = url.split('/').collect(); -// if parts.len() < 3 { -// return Err(ToolError::InvalidRedirectToURL(format!( -// "To-URL '{}' does not have enough parts for locale validation.", -// url -// ))); -// } - -// let to_locale = parts[1]; -// if Locale::from_str(to_locale).is_err() { -// return Err(ToolError::InvalidRedirectToURL(format!( -// "Locale prefix '{}' in To-URL '{}' is not valid.", -// to_locale, url -// ))); -// } - -// Ok(()) -// } - fn is_vanity_redirect_url(url: &str) -> bool { url.strip_prefix('/') .and_then(|url| url.strip_suffix('/')) @@ -892,9 +865,12 @@ mod tests { #[test] fn test_validate_to_url_diff_locale() { + let slugs = vec!["A".to_string()]; + let _docs = DocFixtures::new(&slugs, Locale::EnUs); let url = "/en-US/docs/A"; let locale = Locale::PtBr; let result = validate_to_url(url, locale); + println!("{:?}", result); assert!(result.is_ok()); }