From c1d9bc78ae9c00a213df1a09648111d6092c2b0c Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 9 Jul 2024 17:10:20 +0100 Subject: [PATCH] refactor: move ConfigurationPublic ConfigurationPublic is not a configuration type. It does not belong to the configuration. We don't need to version this type when the configuration changes. It's a type containing a subset of the data contained in the configuration that we want to expose via the API. It happens that those are the fields we want to expose via the API becuase they are the fields we are using in the webapp (Index GUI) but it's not part of the configuration. It's a concrete view of the configration used for other purposes rather than initialize the Index app. In the future, it could be even moved to the API as a API resource. Changing this struct changes the API contract. The contract with the API consumers, not the contract with the Index administrators, the people responsible for setting up the Index and it's configuration. That the reason why it was moved from the config mod to the config service. It's not a problem now, but we should cerate an API resource for this type becuase it should be versioned in the API. WE are using versioning in the API but the type was excluded, meaning it cannot be versioned. --- src/config/mod.rs | 118 +--------------- src/services/settings.rs | 126 +++++++++++++++++- .../web/api/v1/contexts/settings/contract.rs | 2 +- 3 files changed, 127 insertions(+), 119 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index 87154ed8..f27cb7a3 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -2,9 +2,8 @@ pub mod v2; pub mod validator; -use std::str::FromStr; +use std::env; use std::sync::Arc; -use std::{env, fmt}; use camino::Utf8PathBuf; use derive_more::Display; @@ -15,7 +14,6 @@ use serde_with::{serde_as, NoneAsEmptyString}; use thiserror::Error; use tokio::sync::RwLock; use torrust_index_located_error::LocatedError; -use url::Url; use crate::web::api::server::DynError; @@ -301,32 +299,6 @@ impl Configuration { settings_lock.clone() } - pub async fn get_public(&self) -> ConfigurationPublic { - let settings_lock = self.settings.read().await; - - let email_on_signup = match &settings_lock.registration { - Some(registration) => match ®istration.email { - Some(email) => { - if email.required { - EmailOnSignup::Required - } else { - EmailOnSignup::Optional - } - } - None => EmailOnSignup::NotIncluded, - }, - None => EmailOnSignup::NotIncluded, - }; - - ConfigurationPublic { - website_name: settings_lock.website.name.clone(), - tracker_url: settings_lock.tracker.url.clone(), - tracker_listed: settings_lock.tracker.listed, - tracker_private: settings_lock.tracker.private, - email_on_signup, - } - } - pub async fn get_site_name(&self) -> String { let settings_lock = self.settings.read().await; @@ -339,67 +311,12 @@ impl Configuration { } } -/// The public index configuration. -/// There is an endpoint to get this configuration. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -pub struct ConfigurationPublic { - website_name: String, - tracker_url: Url, - tracker_listed: bool, - tracker_private: bool, - email_on_signup: EmailOnSignup, -} - -/// Whether the email is required on signup or not. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(rename_all = "lowercase")] -pub enum EmailOnSignup { - /// The email is required on signup. - Required, - /// The email is optional on signup. - Optional, - /// The email is not allowed on signup. It will only be ignored if provided. - NotIncluded, -} - -impl Default for EmailOnSignup { - fn default() -> Self { - Self::Optional - } -} - -impl fmt::Display for EmailOnSignup { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let display_str = match self { - EmailOnSignup::Required => "required", - EmailOnSignup::Optional => "optional", - EmailOnSignup::NotIncluded => "ignored", - }; - write!(f, "{display_str}") - } -} - -impl FromStr for EmailOnSignup { - type Err = String; - - fn from_str(s: &str) -> Result { - match s.to_lowercase().as_str() { - "required" => Ok(EmailOnSignup::Required), - "optional" => Ok(EmailOnSignup::Optional), - "none" => Ok(EmailOnSignup::NotIncluded), - _ => Err(format!( - "Unknown config 'email_on_signup' option (required, optional, none): {s}" - )), - } - } -} - #[cfg(test)] mod tests { use url::Url; - use crate::config::{ApiToken, Configuration, ConfigurationPublic, EmailOnSignup, Info, SecretKey, Settings}; + use crate::config::{ApiToken, Configuration, Info, SecretKey, Settings}; #[cfg(test)] fn default_config_toml() -> String { @@ -484,37 +401,6 @@ mod tests { assert_eq!(toml, default_config_toml()); } - #[tokio::test] - async fn configuration_should_return_only_public_settings() { - let configuration = Configuration::default(); - let all_settings = configuration.get_all().await; - - let email_on_signup = match &all_settings.registration { - Some(registration) => match ®istration.email { - Some(email) => { - if email.required { - EmailOnSignup::Required - } else { - EmailOnSignup::Optional - } - } - None => EmailOnSignup::NotIncluded, - }, - None => EmailOnSignup::NotIncluded, - }; - - assert_eq!( - configuration.get_public().await, - ConfigurationPublic { - website_name: all_settings.website.name, - tracker_url: all_settings.tracker.url, - tracker_listed: all_settings.tracker.listed, - tracker_private: all_settings.tracker.private, - email_on_signup, - } - ); - } - #[tokio::test] async fn configuration_should_return_the_site_name() { let configuration = Configuration::default(); diff --git a/src/services/settings.rs b/src/services/settings.rs index d587fb9b..8efcc89d 100644 --- a/src/services/settings.rs +++ b/src/services/settings.rs @@ -1,8 +1,13 @@ //! Settings service. +use std::fmt; +use std::str::FromStr; use std::sync::Arc; +use serde::{Deserialize, Serialize}; +use url::Url; + use super::authorization::{self, ACTION}; -use crate::config::{Configuration, ConfigurationPublic, Settings}; +use crate::config::{Configuration, Settings}; use crate::errors::ServiceError; use crate::models::user::UserId; @@ -58,7 +63,8 @@ impl Service { /// /// It returns an error if the user does not have the required permissions. pub async fn get_public(&self) -> ConfigurationPublic { - self.configuration.get_public().await + let settings_lock = self.configuration.get_all().await; + extract_public_settings(&settings_lock) } /// It gets the site name from the settings. @@ -70,3 +76,119 @@ impl Service { self.configuration.get_site_name().await } } + +fn extract_public_settings(settings: &Settings) -> ConfigurationPublic { + let email_on_signup = match &settings.registration { + Some(registration) => match ®istration.email { + Some(email) => { + if email.required { + EmailOnSignup::Required + } else { + EmailOnSignup::Optional + } + } + None => EmailOnSignup::NotIncluded, + }, + None => EmailOnSignup::NotIncluded, + }; + + ConfigurationPublic { + website_name: settings.website.name.clone(), + tracker_url: settings.tracker.url.clone(), + tracker_listed: settings.tracker.listed, + tracker_private: settings.tracker.private, + email_on_signup, + } +} + +/// The public index configuration. +/// There is an endpoint to get this configuration. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct ConfigurationPublic { + website_name: String, + tracker_url: Url, + tracker_listed: bool, + tracker_private: bool, + email_on_signup: EmailOnSignup, +} + +/// Whether the email is required on signup or not. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "lowercase")] +pub enum EmailOnSignup { + /// The email is required on signup. + Required, + /// The email is optional on signup. + Optional, + /// The email is not allowed on signup. It will only be ignored if provided. + NotIncluded, +} + +impl Default for EmailOnSignup { + fn default() -> Self { + Self::Optional + } +} + +impl fmt::Display for EmailOnSignup { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let display_str = match self { + EmailOnSignup::Required => "required", + EmailOnSignup::Optional => "optional", + EmailOnSignup::NotIncluded => "ignored", + }; + write!(f, "{display_str}") + } +} + +impl FromStr for EmailOnSignup { + type Err = String; + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "required" => Ok(EmailOnSignup::Required), + "optional" => Ok(EmailOnSignup::Optional), + "none" => Ok(EmailOnSignup::NotIncluded), + _ => Err(format!( + "Unknown config 'email_on_signup' option (required, optional, none): {s}" + )), + } + } +} + +#[cfg(test)] +mod tests { + use crate::config::Configuration; + use crate::services::settings::{extract_public_settings, ConfigurationPublic, EmailOnSignup}; + + #[tokio::test] + async fn configuration_should_return_only_public_settings() { + let configuration = Configuration::default(); + let all_settings = configuration.get_all().await; + + let email_on_signup = match &all_settings.registration { + Some(registration) => match ®istration.email { + Some(email) => { + if email.required { + EmailOnSignup::Required + } else { + EmailOnSignup::Optional + } + } + None => EmailOnSignup::NotIncluded, + }, + None => EmailOnSignup::NotIncluded, + }; + + assert_eq!( + extract_public_settings(&all_settings), + ConfigurationPublic { + website_name: all_settings.website.name, + tracker_url: all_settings.tracker.url, + tracker_listed: all_settings.tracker.listed, + tracker_private: all_settings.tracker.private, + email_on_signup, + } + ); + } +} diff --git a/tests/e2e/web/api/v1/contexts/settings/contract.rs b/tests/e2e/web/api/v1/contexts/settings/contract.rs index 8b3eb08a..02d30a36 100644 --- a/tests/e2e/web/api/v1/contexts/settings/contract.rs +++ b/tests/e2e/web/api/v1/contexts/settings/contract.rs @@ -1,6 +1,6 @@ //! API contract for `settings` context. -use torrust_index::config::EmailOnSignup; +use torrust_index::services::settings::EmailOnSignup; use torrust_index::web::api; use crate::common::asserts::assert_json_ok_response;