Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration overhaul: make mail-related configuration clearer #670

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Jul 9, 2024

Refactor configuration for options related to email in the registration.

Old version

[auth]
email_on_signup = "optional"

[mail]
email_verification_enabled = false

New version

Registration Disabled:

# [registration]

Registration without email option:

[registration]

Registration optionally including an email address:

[registration]
[registration.email]

Registration with email:

[registration]
[registration.email]
required = true

Registration with optional email, but if email is supplied it is verified:

[registration]
[registration.email]
verified = true

Registration with email and email is verified:

[registration]

[registration.email]
required = true
verified = true

Subtasks

  • Add the new section
  • Use new values in the code
  • Delete old config option

Registration Disabled:

```toml
```

Registration without email option:

```toml
[registration]
```

Registration optionally including an email address:

```toml
[registration]
[registration.email]
```

Registration with  email:

```toml
[registration]
[registration.email]
required = true
```

Registration with optional email, but if email is supplied it is verified:

```toml
[registration]
[registration.email]
verified = true
```

Registration with email and email is verified:

```toml
[registration]
[registration.email]
required = true
verified = true
```

TODO: remove old settings and use the new ones.
@josecelano josecelano self-assigned this Jul 9, 2024
@josecelano josecelano added this to the v3.0.0 milestone Jul 9, 2024
@josecelano josecelano linked an issue Jul 9, 2024 that may be closed by this pull request
The config option:

```toml
[mail]
email_verification_enabled = false
```

was replaced with:

```toml
[registration]

  [registration.email]
  verified = true
```
If the email in the registration form is optional the application should
allow not validate it when it's emtpy. It should only validate the email
when is not empty. We only make sure that if present is a valid email.
@josecelano
Copy link
Member Author

Hi @da2ce7 I think I would change this:

[registration]

  [registration.email] 
  required = true
  verified = true

To this:

[registration]

  [registration.email]
  required = true
  verification_required = true

I think it's less ambiguous, for example, in this real piece of code:

// Fail login if email verification is required and this email is not verified
if let Some(registration) = &settings.registration {
    if let Some(email) = &registration.email {
        if email.verified && !user_profile.email_verified {
            return Err(ServiceError::EmailNotVerified);
        }
    }
}

With the new name would be:

// Fail login if email verification is required and this email is not verified
if let Some(registration) = &settings.registration {
    if let Some(email) = &registration.email {
        if email.verification_required && !user_profile.email_verified {
            return Err(ServiceError::EmailNotVerified);
        }
    }
}

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.
@josecelano josecelano force-pushed the 654-configuration-overhaul-make-mail-related-configuration-clearer branch from c1d9bc7 to a5e745e Compare July 9, 2024 16:34
The endpoint was removed.
@josecelano
Copy link
Member Author

ACK 8d660b8

josecelano added a commit to torrust/torrust-index-gui that referenced this pull request Jul 10, 2024
…email is optional

96ef18e fix: [#585] email field if regsitration form can be empty (Jose Celano)

Pull request description:

  Depends on: torrust/torrust-index#670

  Allow an empty email field in the registration form when email is optional in the configuration.

  To be fully fixed this [PR](torrust/torrust-index#670) has to be merged.

ACKs for top commit:
  josecelano:
    ACK 96ef18e

Tree-SHA512: d9f18e7acceeac8b8016ac8cf12fae5240b78e5f610c7ad522a56f5a5046e95e5b1b50d6c682f43a94cb2c0572968da45d276ca517b1c7e63df33a6ecda7b183
@josecelano josecelano merged commit dc6380d into torrust:develop Jul 10, 2024
13 checks passed
josecelano added a commit to josecelano/torrust-index-gui that referenced this pull request Jul 10, 2024
Relates to: torrust/torrust-index#670

These two config option in the Index TOML config file have been removed:

```toml
[auth]
email_on_signup = "optional"

[mail]
email_verification_enabled = false
```

We need to replace them with:

```toml
[registration]
[registration.email]
```
josecelano added a commit to torrust/torrust-index-gui that referenced this pull request Jul 10, 2024
fc19076 fix: [#587] update Index TOML config file (Jose Celano)

Pull request description:

  Relates to: torrust/torrust-index#670

  These two config option in the Index TOML config file have been removed:

  ```toml
  [auth]
  email_on_signup = "optional"

  [mail]
  email_verification_enabled = false
  ```

  We need to replace them with:

  ```toml
  [registration]
  [registration.email]
  ```

ACKs for top commit:
  josecelano:
    ACK fc19076

Tree-SHA512: d2f02d04c2d08ce3ba29ee5208d720d45db7b1dd376a5f31dc6b22fb6396c7ea9f34577072f56678a9bd77f9d2f75d3989842f7395af49c57c9b26438dca3f12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration overhaul: make mail-related configuration clearer
1 participant