-
Notifications
You must be signed in to change notification settings - Fork 256
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
Sieve #4472
Sieve #4472
Conversation
28b0e6e
to
4e23769
Compare
20bf855
to
a0e33df
Compare
4796be8
to
37893a2
Compare
2c0f621
to
745eb36
Compare
Did some testing today for the update path. Test instance with 3 users and a provisioning configuration. In addition configured a normal email account. Migration added the new fields (to db and provisioning config). Update gone through without issues. State after the update es expected (sieve disabled for all accounts). |
textarea { | ||
width: 100%; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#sieve-text-area { | |
font-size: 14px; | |
line-height: 22px; | |
font-family: monospace; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will evaluate the usage of a nicer editor with syntax highlighting anyway, so let's address this later
if ($config->getSieveEnabled()) { | ||
$account->setSieveUser($config->buildSieveUser($user)); | ||
$account->setSieveHost($config->getSieveHost()); | ||
$account->setSievePort($config->getSievePort()); | ||
$account->setSieveSslMode($config->getSieveSslMode()); | ||
} else { | ||
$account->setSieveUser(null); | ||
$account->setSieveHost(null); | ||
$account->setSievePort(null); | ||
$account->setSieveSslMode(null); | ||
} | ||
|
||
return $account; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($config->getSieveEnabled()) { | |
$account->setSieveUser($config->buildSieveUser($user)); | |
$account->setSieveHost($config->getSieveHost()); | |
$account->setSievePort($config->getSievePort()); | |
$account->setSieveSslMode($config->getSieveSslMode()); | |
} else { | |
$account->setSieveUser(null); | |
$account->setSieveHost(null); | |
$account->setSievePort(null); | |
$account->setSieveSslMode(null); | |
} | |
return $account; | |
} | |
if ($config->getSieveEnabled()) { | |
$account->setSieveUser($config->buildSieveUser($user)); | |
$account->setSieveHost($config->getSieveHost()); | |
$account->setSievePort($config->getSievePort()); | |
$account->setSieveSslMode($config->getSieveSslMode()); | |
return $account; | |
} | |
$account->setSieveUser(null); | |
$account->setSieveHost(null); | |
$account->setSievePort(null); | |
$account->setSieveSslMode(null); | |
return $account; | |
} |
No need to wait to return here, if the condition is fulfilled, we can safely return the set values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no 😉
I think this is one of those cases where the if and the else branch have equal importance. The early return would (to me) signal that it's some error condition or anything very specific.
I would keep the explicit if-else here.
But again, this formatting doesn't change any logic, nor does it make anything faster. I'm fine with either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works fine, no issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works very well!
Looks really good and minimal to me. Well done! Thanks a lot. Dumb question (I didn't apply this patch yet) - has this been tested with multi-account setups? |
Yes. There is a setting per account. |
37525c9
to
efc2bb5
Compare
* Expose managesieve port * Add sieve client factory * Add support for sieve to provisioning * Refactor test connectivity logic and add sieve. * Add support for sieve to provisioning * Add sieve to account form * Add debug logger for ManageSieve * Add api to get and update active script * Add error for managesieve exception * Add text editor to update existing script Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
efc2bb5
to
82ab9d0
Compare
Todo:
sieveEnabled = false
to existing provisioning configurationsNice to have:
CI:
Build fails because the sieve integration server uses self signed certificates.We need horde/ManageSieve#1 or apply the patch on CI.
Screenshots: