Add switch to manage the smarthost between cluster and Nextcloud#175
Open
Add switch to manage the smarthost between cluster and Nextcloud#175
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a UI toggle to allow administrators to choose whether email sending for Nextcloud should be managed by the cluster's smarthost or configured directly within Nextcloud's admin panel. This provides flexibility for installations that need custom email configurations in Nextcloud.
Changes:
- Added a toggle switch in the Settings UI to control the
internal_smarthostsetting - Added environment variable
INTERNAL_SMARTHOSTto track whether Nextcloud manages its own email settings - Modified
discover-smarthostandsetup-smtpto respect the internal smarthost flag - Added migration script to detect and preserve existing email configuration during updates
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/Settings.vue | Added NsToggle component for internal_smarthost setting with data binding to backend |
| ui/public/i18n/en/translation.json | Added English translation label for the internal smarthost toggle |
| imageroot/update-module.d/17smarthost_switch | Migration script to detect whether cluster or Nextcloud was managing email in existing installations |
| imageroot/bin/setup-smtp | Modified to only apply cluster smarthost settings when INTERNAL_SMARTHOST is False, and to clean up Nextcloud email config when disabled |
| imageroot/bin/discover-smarthost | Modified to skip writing smarthost.env when internal_smarthost is enabled |
| imageroot/actions/get-configuration/20read | Added default value for internal_smarthost field |
| imageroot/actions/configure-module/20configure | Added INTERNAL_SMARTHOST environment variable with default value |
Comments suppressed due to low confidence (5)
imageroot/update-module.d/17smarthost_switch:25
- The occget function is called without checking if the Nextcloud container is running. If Nextcloud is not running during the update, this subprocess call will fail and the update script will crash. Consider adding error handling or checking container status before calling occget.
smtphost = occget(["config:system:get","mail_smtphost"])
ui/public/i18n/en/translation.json:50
- The label "Manage email sending from Nextcloud admin panel" is unclear about what enabling/disabling this toggle does. When enabled, does Nextcloud manage email (internal smarthost), or does the cluster manage it? Consider a clearer label such as "Use Nextcloud email settings" or "Configure email from Nextcloud admin panel" to make it clear that enabling this means Nextcloud manages its own email settings.
"internal_smarthost": "Manage email sending from Nextcloud admin panel"
imageroot/update-module.d/17smarthost_switch:29
- Missing space after equals sign. Should be 'smtpsmarthost = smarthost['SMTP_HOST']' for consistency with Python PEP 8 style guidelines and the rest of the codebase.
smtpsmarthost=smarthost['SMTP_HOST']
imageroot/update-module.d/17smarthost_switch:36
- The migration logic has an edge case: if both mail_smtphost and SMTP_HOST are empty/unset, they will match and INTERNAL_SMARTHOST will be set to False (cluster management). However, in this case, there's no cluster smarthost configured, so it's unclear which mode should be selected. Consider adding a check to handle the case where both values are empty, and default to INTERNAL_SMARTHOST=True in that case, since the cluster isn't actually managing email.
if smtphost == smtpsmarthost:
config['INTERNAL_SMARTHOST']=False
agent.write_envfile('config.env', config)
imageroot/update-module.d/17smarthost_switch:31
- Missing space after equals sign. Should be 'smtpsmarthost = ""' for consistency with Python PEP 8 style guidelines and the rest of the codebase.
smtpsmarthost=""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or 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
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.
NethServer/dev#7871