Add switch to manage the smarthost between cluster and Nextcloud#175
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
imageroot/update-module.d/17smarthost_switch:16
- As in
setup-smtp,occget()usesp.stdout.decode()[:-1], which will drop the last character even when no trailing newline is present (potentially corrupting the smtphost value used for comparison). Preferrstrip('\n')/strip()instead of slicing.
def occget(args):
cmd = ['podman', 'exec', '--user', 'www-data', 'nextcloud-app', 'php', './occ']
p = subprocess.run(cmd + args, capture_output=True)
return (p.stdout.decode()[:-1])
ui/public/i18n/en/translation.json:50
- Only the English locale adds
settings.internal_smarthost. Other existing locales (e.g.ui/public/i18n/it/translation.json) don't have this key, so users with those languages will see the raw key instead of a label. Please add the new string to the other translation files (or ensure a per-key fallback mechanism exists).
"host_format": "Must be a valid fully qualified domain name",
"internal_smarthost": "Manage email sending from Nextcloud admin panel"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DavidePrincipi
left a comment
There was a problem hiding this comment.
Seems good, but what happens on first app configuration, when containers aren't running? Please confirm that helper commands do not raise errors.
| p = subprocess.run(cmd + args, capture_output=True) | ||
| return (p.stdout.decode().rstrip()) | ||
|
|
||
| smarthost = agent.read_envfile("smarthost.env") |
There was a problem hiding this comment.
This should go after the guard block, lines 24-25.
| return (p.stdout.decode().rstrip()) | ||
|
|
||
| smarthost= agent.read_envfile("smarthost.env") | ||
| smtphost= occget(["config:system:get","mail_smtphost"]) |
There was a problem hiding this comment.
If nextcloud-app container is not running, this command will return an empty value. Is it handled correctly?
|
|
||
| domain = config['host'].split('.',1)[1] | ||
|
|
||
| with open(envfile + ".tmp", "w") as efp: |
There was a problem hiding this comment.
Since core 3.18 we have agent.safe_open() that fully handles this typical .tmp file + rename pattern, with a safe fsync() call. What about using it here?
| "host_pattern": "Must be a valid fully qualified domain name", | ||
| "host_format": "Must be a valid fully qualified domain name" | ||
| "host_format": "Must be a valid fully qualified domain name", | ||
| "internal_smarthost": "Manage email sending from Nextcloud admin panel" |
There was a problem hiding this comment.
Hint for next translation.json file modification: append on top to avoid changing two lines. It reduces patch size (and possibly merges conflicts).
NethServer/dev#7871