Skip to content

Commit

Permalink
refactor: move ConfigurationPublic
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
josecelano committed Jul 9, 2024
1 parent 4fbeead commit a5e745e
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 120 deletions.
118 changes: 2 additions & 116 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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 &registration.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;

Expand All @@ -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<Self, Self::Err> {
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 {
Expand Down Expand Up @@ -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 &registration.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();
Expand Down
126 changes: 124 additions & 2 deletions src/services/settings.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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.
Expand All @@ -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 &registration.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<Self, Self::Err> {
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 &registration.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,
}
);
}
}
2 changes: 1 addition & 1 deletion src/web/api/server/v1/contexts/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@
//!
//! **Resource**
//!
//! Refer to the [`ConfigurationPublic`](crate::config::ConfigurationPublic)
//! Refer to the [`ConfigurationPublic`](crate::services::settings::ConfigurationPublic)
//! struct for more information about the response attributes.
pub mod handlers;
pub mod routes;
2 changes: 1 addition & 1 deletion tests/e2e/web/api/v1/contexts/settings/contract.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down

0 comments on commit a5e745e

Please sign in to comment.