From 3acfcbe4332333a3ee1eca9bfa67e2e30a0693e6 Mon Sep 17 00:00:00 2001 From: Coko <91132775+Coko7@users.noreply.github.com> Date: Tue, 2 Jun 2026 07:45:17 +0200 Subject: [PATCH] fix(edit): add --editor flag and validate editor before launching Add a `-e`/`--editor` CLI flag to override the editor used by the edit command. Editor resolution now prefers the flag, falls back to $EDITOR, and fails explicitly when neither is set (instead of silently defaulting to vim). The resolved editor is validated via `which` before launch to give a clear error on missing executables. Add unit tests for resolve_editor, try_generate_edit_diffs, and display helpers, and add tests for PrintableActivityLog row rendering. --- Cargo.lock | 10 +++ Cargo.toml | 1 + src/cli/args.rs | 4 ++ src/commands/edit.rs | 141 ++++++++++++++++++++++++++++++++++++- src/models/activity_log.rs | 66 +++++++++++++++++ src/utils/display.rs | 46 ++++++++++++ tests/cli_delete.rs | 15 ++++ tests/cli_get.rs | 14 ++++ tests/cli_list.rs | 25 +++++++ tests/cli_new.rs | 82 +++++++++++++++++++++ tests/cli_pause.rs | 30 ++++++++ tests/cli_start.rs | 33 +++++++++ 12 files changed, 465 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8798c4..e649195 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -130,6 +130,7 @@ dependencies = [ "tabular", "tempfile", "toml", + "which", "yansi", ] @@ -1172,6 +1173,15 @@ dependencies = [ "semver", ] +[[package]] +name = "which" +version = "8.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "81995fafaaaf6ae47a7d0cc83c67caf92aeb7e5331650ae6ff856f7c0c60c459" +dependencies = [ + "libc", +] + [[package]] name = "windows-core" version = "0.62.2" diff --git a/Cargo.toml b/Cargo.toml index e631664..bc794f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ boat-lib = "0.5.0" tabular = { version = "0.2.0", features = ["ansi-cell"] } yansi = "1.0.1" dialoguer = "0.12.0" +which = "8.0.2" [dev-dependencies] assert_cmd = "2.0" diff --git a/src/cli/args.rs b/src/cli/args.rs index 6bc3481..da8c3d5 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -287,6 +287,10 @@ pub struct EditLogsArgs { )] pub period: Option, + /// Specify the editor program to use + #[arg(short = 'e', long = "editor")] + pub editor: Option, + /// Include instruction comments in the editable file #[arg(short = 'i', long = "with-instructions", alias = "with-instr", action = ArgAction::SetTrue, conflicts_with = "hide_instructions")] pub show_instructions: bool, diff --git a/src/commands/edit.rs b/src/commands/edit.rs index b3c9d1b..50f74f3 100644 --- a/src/commands/edit.rs +++ b/src/commands/edit.rs @@ -56,7 +56,14 @@ pub fn edit( let default_content = boat_data.to_csv_str(include_instructions, include_activity_definitions); let edit_file_path = create_tmp_edit_file(&default_content)?; - let editor = env::var("EDITOR").unwrap_or_else(|_| "vim".to_string()); + let editor = args + .editor + .clone() + .or_else(|| env::var("EDITOR").ok()) + .context("No editor specified and EDITOR is not set")?; + + resolve_editor(&editor)?; + info!("launching editor: {editor}"); let status = Command::new(editor).arg(&edit_file_path).status()?; ensure!( @@ -137,6 +144,17 @@ struct EditLogDiff { ends_at_new: Option>>, } +fn resolve_editor(editor: &str) -> Result<()> { + if editor.is_empty() { + bail!( + "Default text editor not found. Please refer to your shell documentation to set your EDITOR environment variable." + ); + } + + which::which(editor).with_context(|| format!("EDITOR command not found: {editor}"))?; + Ok(()) +} + fn pretty_print_edit_diffs(diffs: &[EditLogDiff]) { println!("Detected changes:"); for diff in diffs { @@ -391,10 +409,13 @@ fn convert_modified_content_to_log_lines(content: &str) -> Result Result { - info!("creating temporary file for editing"); let tmp_dir = env::temp_dir(); let file_path = tmp_dir.join("boat_edit_logs_tmp.csv"); fs::write(&file_path, content)?; + info!( + "created temporary file for editing: {}", + file_path.display() + ); Ok(file_path) } @@ -478,4 +499,120 @@ mod tests { let csv = "1, 10, not-a-date, 2024-06-01 11:00"; assert!(convert_modified_content_to_log_lines(csv).is_err()); } + + // --- resolve_editor --- + + #[test] + fn resolve_editor_empty_string_fails() { + assert!(resolve_editor("").is_err()); + } + + #[test] + fn resolve_editor_nonexistent_command_fails() { + assert!(resolve_editor("definitely-not-a-real-editor-xyz").is_err()); + } + + // --- try_generate_edit_diffs error paths --- + + #[test] + fn try_generate_edit_diffs_length_mismatch_fails() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![ + make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00")), + make_log(2, 1, "2024-06-01 12:00", Some("2024-06-01 13:00")), + ]; + assert!(try_generate_edit_diffs(&edited, &orig).is_err()); + } + + #[test] + fn try_generate_edit_diffs_multiple_open_ended_logs_fails() { + let orig = vec![ + make_log(1, 1, "2024-06-01 10:00", None), + make_log(2, 1, "2024-06-01 12:00", None), + ]; + assert!(try_generate_edit_diffs(&orig, &orig).is_err()); + } + + #[test] + fn try_generate_edit_diffs_unknown_log_id_fails() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![make_log( + 99, + 1, + "2024-06-01 10:00", + Some("2024-06-01 11:00"), + )]; + assert!(try_generate_edit_diffs(&edited, &orig).is_err()); + } + + #[test] + fn try_generate_edit_diffs_activity_id_changed_fails() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![make_log( + 1, + 999, + "2024-06-01 10:00", + Some("2024-06-01 11:00"), + )]; + assert!(try_generate_edit_diffs(&edited, &orig).is_err()); + } + + #[test] + fn try_generate_edit_diffs_starts_at_after_ends_at_fails() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![make_log(1, 1, "2024-06-01 12:00", Some("2024-06-01 11:00"))]; + assert!(try_generate_edit_diffs(&edited, &orig).is_err()); + } + + // --- try_generate_edit_diffs happy paths --- + + #[test] + fn try_generate_edit_diffs_no_change_returns_empty() { + let logs = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let diffs = try_generate_edit_diffs(&logs, &logs).unwrap(); + assert!(diffs.is_empty()); + } + + #[test] + fn try_generate_edit_diffs_detects_ends_at_change() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 12:00"))]; + let diffs = try_generate_edit_diffs(&edited, &orig).unwrap(); + assert_eq!(diffs.len(), 1); + assert!(diffs[0].ends_at_new.is_some()); + assert!(diffs[0].starts_at_new.is_none()); + } + + #[test] + fn try_generate_edit_diffs_detects_log_closed_to_open() { + let orig = vec![make_log(1, 1, "2024-06-01 10:00", Some("2024-06-01 11:00"))]; + let edited = vec![make_log(1, 1, "2024-06-01 10:00", None)]; + let diffs = try_generate_edit_diffs(&edited, &orig).unwrap(); + assert_eq!(diffs.len(), 1); + // ends_at_new is Some(None) — the log was re-opened + assert_eq!(diffs[0].ends_at_new, Some(None)); + } + + // --- date_time_opt_loose_eq --- + + #[test] + fn date_time_opt_loose_eq_both_none_are_equal() { + let none: Option> = None; + assert!(date_time_opt_loose_eq(&none, &none).unwrap()); + } + + #[test] + fn date_time_opt_loose_eq_some_and_none_are_not_equal() { + let some = Some(Utc.with_ymd_and_hms(2024, 6, 1, 10, 0, 0).unwrap()); + let none = None; + assert!(!date_time_opt_loose_eq(&some, &none).unwrap()); + assert!(!date_time_opt_loose_eq(&none, &some).unwrap()); + } + + #[test] + fn date_time_opt_loose_eq_different_minutes_are_not_equal() { + let dt1 = Some(Utc.with_ymd_and_hms(2024, 6, 1, 10, 0, 0).unwrap()); + let dt2 = Some(Utc.with_ymd_and_hms(2024, 6, 1, 10, 1, 0).unwrap()); + assert!(!date_time_opt_loose_eq(&dt1, &dt2).unwrap()); + } } diff --git a/src/models/activity_log.rs b/src/models/activity_log.rs index 27fdc3a..f309bba 100644 --- a/src/models/activity_log.rs +++ b/src/models/activity_log.rs @@ -68,3 +68,69 @@ impl RowPrintable for PrintableActivityLog { } } } + +#[cfg(test)] +mod tests { + use super::*; + use boat_lib::models::log::Log as DatabaseLog; + use chrono::{TimeZone, Utc}; + use std::collections::HashSet; + + fn make_activity() -> SimpleActivity { + SimpleActivity { + id: 7, + name: "testing".to_string(), + description: Some("a desc".to_string()), + tags: HashSet::new(), + } + } + + fn make_log(ends_after_secs: Option) -> DatabaseLog { + let start = Utc.with_ymd_and_hms(2024, 4, 15, 10, 0, 0).unwrap(); + DatabaseLog { + id: 42, + activity_id: 7, + starts_at: start, + ends_at: ends_after_secs.map(|s| start + chrono::Duration::seconds(s)), + } + } + + #[test] + fn row_values_open_log_shows_dash_for_end() { + let pal = PrintableActivityLog::from_activity_and_log(&make_activity(), &make_log(None)); + assert_eq!(pal.row_values()[5], "-"); + } + + #[test] + fn row_values_closed_log_has_non_empty_end() { + let pal = + PrintableActivityLog::from_activity_and_log(&make_activity(), &make_log(Some(3600))); + let end_col = &pal.row_values()[5]; + assert_ne!(end_col, "-"); + assert!(!end_col.is_empty()); + } + + #[test] + fn row_values_id_and_name_match_activity() { + let pal = + PrintableActivityLog::from_activity_and_log(&make_activity(), &make_log(Some(3600))); + let values = pal.row_values(); + assert_eq!(values[0], "7"); + assert_eq!(values[1], "testing"); + } + + #[test] + fn style_cell_closed_log_returns_value_unchanged() { + let pal = + PrintableActivityLog::from_activity_and_log(&make_activity(), &make_log(Some(3600))); + let val = "unchanged".to_string(); + assert_eq!(pal.style_cell(val.clone()), val); + } + + #[test] + fn style_cell_open_log_result_contains_original_value() { + let pal = PrintableActivityLog::from_activity_and_log(&make_activity(), &make_log(None)); + let val = "styled".to_string(); + assert!(pal.style_cell(val.clone()).contains(&val)); + } +} diff --git a/src/utils/display.rs b/src/utils/display.rs index 79713f9..83f914d 100644 --- a/src/utils/display.rs +++ b/src/utils/display.rs @@ -190,3 +190,49 @@ pub fn current_activity_msg(activity: &DatabaseActivity) -> Result { pub fn no_current_act_msg() -> String { "no current activity".to_string() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn get_group_by_display_values_none_returns_all() { + let (label, tooltip) = get_group_by_display_values(GroupBy::None, "anything").unwrap(); + assert_eq!(label, "ALL"); + assert!(tooltip.is_none()); + } + + #[test] + fn get_group_by_display_values_year_returns_key() { + let (label, tooltip) = get_group_by_display_values(GroupBy::Year, "2024").unwrap(); + assert_eq!(label, "2024"); + assert!(tooltip.is_none()); + } + + #[test] + fn get_group_by_display_values_month_formats_name() { + let (label, tooltip) = get_group_by_display_values(GroupBy::Month, "2024-04").unwrap(); + assert_eq!(label, "April 2024"); + assert!(tooltip.is_none()); + } + + #[test] + fn get_group_by_display_values_month_invalid_key_fails() { + assert!(get_group_by_display_values(GroupBy::Month, "not-a-month").is_err()); + } + + #[test] + fn get_group_by_display_values_week_label_and_date_range() { + let (label, tooltip) = get_group_by_display_values(GroupBy::Week, "2024-W10").unwrap(); + assert_eq!(label, "Week 10"); + // 2024 starts on a Monday, so week 10 runs Mar 04–Mar 10 + let tooltip = tooltip.unwrap(); + assert!(tooltip.contains("Mar 04, 2024"), "got: {tooltip}"); + assert!(tooltip.contains("Mar 10, 2024"), "got: {tooltip}"); + } + + #[test] + fn get_group_by_display_values_week_invalid_key_fails() { + assert!(get_group_by_display_values(GroupBy::Week, "not-a-week").is_err()); + } +} diff --git a/tests/cli_delete.rs b/tests/cli_delete.rs index e4991ce..b36fa1b 100644 --- a/tests/cli_delete.rs +++ b/tests/cli_delete.rs @@ -49,3 +49,18 @@ fn delete_with_missing_id_arg_fails() -> Result<()> { Ok(()) } + +#[test] +fn delete_running_activity_succeeds() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "ActiveTask", "--start-now"], &config_path).success(); + + run_boat(["delete", "1", "--no-confirm"], &config_path).success(); + + run_boat(["list", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("ActiveTask").not()); + + Ok(()) +} diff --git a/tests/cli_get.rs b/tests/cli_get.rs index 1600c83..c199ef1 100644 --- a/tests/cli_get.rs +++ b/tests/cli_get.rs @@ -41,3 +41,17 @@ fn get_when_no_current_activity_fails() -> Result<()> { Ok(()) } + +#[test] +fn get_after_pause_fails() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "WasRunning", "--start-now"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + run_boat(["get", "--json"], &config_path) + .failure() + .stderr(predicates::str::contains("no current activity")); + + Ok(()) +} diff --git a/tests/cli_list.rs b/tests/cli_list.rs index b2095ee..9e8a397 100644 --- a/tests/cli_list.rs +++ b/tests/cli_list.rs @@ -6,6 +6,31 @@ use crate::utils::{cli_args_for_temp, run_boat}; mod utils; +#[test] +fn list_with_no_activities_shows_no_data() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["list"], config_path) + .success() + .stdout(predicates::str::contains("no available data")); + + Ok(()) +} + +#[test] +fn list_shows_activity_name() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "ListedTask", "--start-now"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + run_boat(["list"], &config_path) + .success() + .stdout(predicates::str::contains("ListedTask")); + + Ok(()) +} + #[test] fn list_with_invalid_date_input_fails() -> Result<()> { let (_tmp, config_path) = cli_args_for_temp()?; diff --git a/tests/cli_new.rs b/tests/cli_new.rs index 025e998..80f023b 100644 --- a/tests/cli_new.rs +++ b/tests/cli_new.rs @@ -16,3 +16,85 @@ fn new_without_name_fails() -> Result<()> { Ok(()) } + +#[test] +fn new_with_name_succeeds() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "MyTask"], &config_path).success(); + + // Start and pause so it has a log and appears in list output + run_boat(["start", "1"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + run_boat(["list", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("MyTask")); + + Ok(()) +} + +#[test] +fn new_with_start_now_starts_activity() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "AutoStarted", "--start-now"], &config_path).success(); + + run_boat(["get", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("AutoStarted")); + + Ok(()) +} + +#[test] +fn new_with_no_auto_start_does_not_start() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "NotStarted", "--no-start-now"], &config_path).success(); + + run_boat(["get", "--json"], &config_path) + .failure() + .stderr(predicates::str::contains("no current activity")); + + Ok(()) +} + +#[test] +fn new_with_tags_creates_tagged_activity() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "TaggedTask", "--tags", "rust,cli"], &config_path).success(); + run_boat(["start", "1"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + run_boat(["list", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("rust").and(predicates::str::contains("cli"))); + + Ok(()) +} + +#[test] +fn new_with_description_creates_described_activity() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat( + [ + "new", + "DescribedTask", + "--description", + "a very useful task", + ], + &config_path, + ) + .success(); + run_boat(["start", "1"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + run_boat(["list", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("a very useful task")); + + Ok(()) +} diff --git a/tests/cli_pause.rs b/tests/cli_pause.rs index 9ebd03a..a23119c 100644 --- a/tests/cli_pause.rs +++ b/tests/cli_pause.rs @@ -15,3 +15,33 @@ fn pause_when_nothing_running_fails() -> Result<()> { Ok(()) } + +#[test] +fn pause_running_activity_succeeds() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "PauseMe", "--start-now"], &config_path).success(); + + run_boat(["pause"], &config_path) + .success() + .stdout(predicates::str::contains("paused")); + + run_boat(["get", "--json"], &config_path) + .failure() + .stderr(predicates::str::contains("no current activity")); + + Ok(()) +} + +#[test] +fn pause_outputs_activity_name() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "NamedActivity", "--start-now"], &config_path).success(); + + run_boat(["pause"], &config_path) + .success() + .stdout(predicates::str::contains("NamedActivity")); + + Ok(()) +} diff --git a/tests/cli_start.rs b/tests/cli_start.rs index 973171b..4b20bf0 100644 --- a/tests/cli_start.rs +++ b/tests/cli_start.rs @@ -43,3 +43,36 @@ fn start_when_other_activity_is_running_suceeds() -> Result<()> { Ok(()) } + +#[test] +fn start_resumes_paused_activity() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "ResumeMe"], &config_path).success(); + run_boat(["start", "1"], &config_path).success(); + run_boat(["pause"], &config_path).success(); + + // Resuming the paused activity should succeed and make it current again + run_boat(["start", "1"], &config_path).success(); + + run_boat(["get", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("ResumeMe")); + + Ok(()) +} + +#[test] +fn start_by_name_succeeds() -> Result<()> { + let (_tmp, config_path) = cli_args_for_temp()?; + + run_boat(["new", "ByName"], &config_path).success(); + + run_boat(["start", "ByName"], &config_path).success(); + + run_boat(["get", "--json"], &config_path) + .success() + .stdout(predicates::str::contains("ByName")); + + Ok(()) +}