Re: [pmg-devel] [PATCH pmg-yew-quarantine-gui] execute actions when opening gui from a link from quarantine mails

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 29.10.25 um 15:45 schrieb Dominik Csapak:
> The PMG quarantine mails can contain links that should do some actions
> directly, e.g. deliver or delete a mail. This works for our desktop gui
> and worked in the old Framework7 mobile ui, but was forgotten to be
> implemented in the new rust based gui.
> 
> This patch implements it in `SpamList` in the 'create' method.
> 
> To make it work properly we have to omit the `location.replace` when
> doing a login, otherwise we lose the get parameters and refresh the
> page. Instead remove the ticket parameter (when we extrat that) with
> the browsers `history` api, and the rest when we execute the action.
> 
> A SnackBar is also added on successful execution of an action.
> 
> Parts of this patch are take from Stoiko's first approach[0].
> 
> Note that while this patch enables the actions when entering from a
> quarantine link, it won't show a SnackBar (neither on error or success)
> due to our `CatalogLoader` behavior. A fix for that is on the yew-devel
> list [1].
> 
> 0: https://lore.proxmox.com/pmg-devel/20251028163628.79739-1-s.ivanov@proxmox.com/
> 1: https://lore.proxmox.com/yew-devel/20251029143617.18134-1-d.csapak@proxmox.com/T/#u

Looks OK to me in general principle, some comments (mostly nits and naming)
inline though, and would also like to get a T-b, e.g. from Stoiko (but I'm
not picky ;-))

> 
> Co-developed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/main.rs       | 22 +++++++++---
>  src/page_login.rs |  5 +++
>  src/spam_list.rs  | 89 +++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 97 insertions(+), 19 deletions(-)
> 
> diff --git a/src/main.rs b/src/main.rs
> index 8cd8a6b..bb0ecc1 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -14,8 +14,8 @@ pub use page_not_found::PageNotFound;
>  mod page_login;
>  pub use page_login::PageLogin;
>  
> -use anyhow::Error;
> -use gloo_utils::{document, format::JsValueSerdeExt};
> +use anyhow::{format_err, Error};
> +use gloo_utils::format::JsValueSerdeExt;
>  use serde::Deserialize;
>  use serde_json::{json, Value};
>  use wasm_bindgen::JsValue;
> @@ -158,9 +158,6 @@ impl Component for PmgQuarantineApp {
>          match msg {
>              Msg::Login(info) => {
>                  self.login_info = Some(info.clone());
> -                let document = document();
> -                let location = document.location().unwrap();
> -                let _ = location.replace(&location.pathname().unwrap());
>              }
>              Msg::Logout => self.login_info = None,
>          }
> @@ -187,6 +184,21 @@ impl std::fmt::Display for MailAction {
>      }
>  }
>  
> +impl std::str::FromStr for MailAction {
> +    type Err = anyhow::Error;
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s {
> +            "deliver" => Ok(MailAction::Deliver),
> +            "delete" => Ok(MailAction::Delete),
> +            "welcomelist" => Ok(MailAction::Welcomelist),
> +            "whitelist" => Ok(MailAction::Welcomelist),

tiny nit: could be written as alternatives to better clarify that it's the same, i.e.:

"welcomelist" | "whitelist" => Ok(MailAction::Welcomelist),

> +            "blocklist" => Ok(MailAction::Blocklist),
> +            "blacklist" => Ok(MailAction::Blocklist),

similar nit above.

> +            _ => Err(format_err!("unknown quarantine action")),
> +        }
> +    }
> +}
> +
>  pub(crate) async fn mail_action(id: &str, action: MailAction) -> Result<Value, Error> {
>      let param = json!({
>          "action": action.to_string(),
> diff --git a/src/page_login.rs b/src/page_login.rs
> index d285209..f1e3406 100644
> --- a/src/page_login.rs
> +++ b/src/page_login.rs
> @@ -20,6 +20,8 @@ use proxmox_yew_comp::{
>      http_login, start_ticket_refresh_loop, stop_ticket_refresh_loop, LoginPanel,
>  };
>  
> +use crate::spam_list::remove_get_parameter;
> +
>  #[derive(Properties, PartialEq)]
>  pub struct PageLogin {
>      #[prop_or_default]
> @@ -90,6 +92,9 @@ impl Component for PmgPageLogin {
>                          Self::ticket_login(ctx, username.to_string(), ticket.to_string());
>                          stop_ticket_refresh_loop();
>                      }
> +                    if let Err(err) = remove_get_parameter(Some("ticket")) {
> +                        log::error!("could not remove ticket parameter: {err}");
> +                    }
>                  }
>              }
>          }
> diff --git a/src/spam_list.rs b/src/spam_list.rs
> index b870e12..0af31c6 100644
> --- a/src/spam_list.rs
> +++ b/src/spam_list.rs
> @@ -1,10 +1,14 @@
> -use anyhow::Error;
> -use wasm_bindgen::JsValue;
> +use std::{rc::Rc, str::FromStr};
>  
> -use core::clone::Clone;
> +use anyhow::{format_err, Error};
> +use gloo_utils::{document, window};
>  use js_sys::Date;
> +use serde::{Deserialize, Serialize};
>  use serde_json::Value;
> -use std::rc::Rc;
> +use url::{form_urlencoded, Url};
> +use wasm_bindgen::JsValue;
> +use yew::html::{IntoEventCallback, IntoPropValue};
> +use yew::virtual_dom::{VComp, VNode};
>  
>  use pwt::{
>      css::{AlignItems, ColorScheme, FlexFit, Opacity, Overflow},
> @@ -12,17 +16,10 @@ use pwt::{
>      touch::{Slidable, SlidableAction, SnackBar, SnackBarContextExt},
>      widget::{error_message, Container, Fa, List, ListTile, Progress, Row},
>  };
> -use yew::{
> -    html::{IntoEventCallback, IntoPropValue},
> -    virtual_dom::{VComp, VNode},
> -};
> -//use yew::html::IntoEventCallback;
>  
>  use proxmox_yew_comp::http_get;
>  use pwt::widget::Column;
>  
> -use serde::{Deserialize, Serialize};
> -
>  use crate::{mail_action, MailAction};
>  
>  #[derive(Copy, Clone, Serialize, Default, PartialEq)]
> @@ -110,6 +107,19 @@ impl Component for PmgSpamList {
>  
>      fn create(ctx: &Context<Self>) -> Self {
>          let me = Self { data: None };
> +
> +        match extract_mail_action_from_get() {
> +            Ok(None) => {}
> +            Ok(Some((id, action))) => {
> +                ctx.link().send_message(Msg::Action(id, action));
> +            }
> +            Err(err) => {
> +                ctx.link().show_snackbar(
> +                    SnackBar::new().message(format!("could not call action: {err}")),

Would do s/call/execute/, as calling is IMO not that easy to understand.

> +                );
> +            }
> +        }
> +
>          me.load(ctx);
>          me
>      }
> @@ -145,9 +155,11 @@ impl Component for PmgSpamList {
>              Msg::Action(id, action) => {
>                  let link = ctx.link().clone();
>                  wasm_bindgen_futures::spawn_local(async move {
> -                    if let Err(err) = mail_action(&id, action).await {
> -                        link.show_snackbar(SnackBar::new().message(err.to_string()));
> -                    }
> +                    let msg = match mail_action(&id, action).await {
> +                        Ok(_) => tr!("Action '{0}' successful", action),
> +                        Err(err) => err.to_string(),
> +                    };
> +                    link.show_snackbar(SnackBar::new().message(msg));
>                      link.send_message(Msg::Reload);
>                  });
>                  return false;
> @@ -299,3 +311,52 @@ fn epoch_to_date(epoch: i64) -> String {
>          date.get_date()
>      )
>  }
> +
> +fn extract_mail_action_from_get() -> Result<Option<(String, MailAction)>, Error> {
> +    let document = document();
> +    let location = document.location().unwrap();
> +    let search = location.search().unwrap();
> +    let param = web_sys::UrlSearchParams::new_with_str(&search).unwrap();
> +
> +    remove_get_parameter(None)?;
> +
> +    if let (Some(id), Some(action)) = (param.get("cselect"), param.get("action")) {
> +        let action = MailAction::from_str(&action)?;
> +        return Ok(Some((id, action)));
> +    }
> +    Ok(None)
> +}
> +
> +/// Removes `name` parameter from the get values via the browser `history` object.
> +///
> +/// If no name is given, removes all parameters.
> +pub fn remove_get_parameter(name: Option<&str>) -> Result<(), Error> {

"get" is a bit ambiguous here IMO as these parameters in the URL/location can
be send on any request. Maybe better use "search" or "query".

Also why delete here and extract above? In the backend we also got a quite frequently
used "extract_param" method for something that deletes and returns a specific parameter,
so might be fitting here. E.g.:

fn extract_query_parameter

Or even

fn extract_location_query_parameter


And for above extract_mail_action_from_get then either extract_mail_action_from_query or
extract_mail_action_from_location_query, respectively. Also s/get/query/ in the error
log message on call site.

> +    let location = window().location();
> +    let history = window().history().unwrap();
> +
> +    let mut url = Url::parse(
> +        &location
> +            .href()
> +            .map_err(|err| format_err!("could not get location: {err:?}"))?,
> +    )?;
> +    if let Some(name) = name {
> +        let mut query = form_urlencoded::Serializer::new(String::new());
> +        for (key, value) in url.query_pairs() {
> +            if key == name {
> +                continue;
> +            }
> +            query.append_pair(&key, &value);
> +        }
> +
> +        url.set_query(Some(&query.finish()));
> +    } else {
> +        url.set_query(None);
> +    }
> +
> +    // build the new url without get parameters
> +    history
> +        .replace_state_with_url(&JsValue::null(), "", Some(url.as_str()))
> +        .map_err(|err| format_err!("could not set url: {err:?}"))?;
> +
> +    Ok(())
> +}



_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel