-
Notifications
You must be signed in to change notification settings - Fork 19
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
Merged
josecelano
merged 6 commits into
torrust:develop
from
josecelano:654-configuration-overhaul-make-mail-related-configuration-clearer
Jul 10, 2024
Merged
Configuration overhaul: make mail-related configuration clearer #670
josecelano
merged 6 commits into
torrust:develop
from
josecelano:654-configuration-overhaul-make-mail-related-configuration-clearer
Jul 10, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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) = ®istration.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) = ®istration.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
force-pushed
the
654-configuration-overhaul-make-mail-related-configuration-clearer
branch
from
July 9, 2024 16:34
c1d9bc7
to
a5e745e
Compare
The endpoint was removed.
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
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
This was referenced Jul 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor configuration for options related to email in the registration.
Old version
New version
Registration Disabled:
# [registration]
Registration without email option:
[registration]
Registration optionally including an email address:
Registration with email:
Registration with optional email, but if email is supplied it is verified:
Registration with email and email is verified:
Subtasks