Feature: Configurable default spam threshold used for new users #2328
Conversation
|
Thanks for submitting this pull request. bors try Note: if this build fails, read this. |
tryBuild succeeded: |
enginefeeder101
changed the title
There was a problem hiding this comment.
Thank you for submitting this PR. This PR is worthwhile. I can imagine that administrators want the possibility for setting a default threshold. But the PR requires some changes. We normally assign all environment variables to a global dictionary (app.config). This dictionary is used in other parts of Mailu.
So don't use the env var directly in models.py. Add the value to the global config.
In configuration.py add DEFAULT_SPAM_THRESHOLD to the default config (line 7+):
'DEFAULT_SPAM_THRESHOLD': 80,
In Class ConfigManager, in function init_app (see line 136+), you can add the code for assigning the value to the config:
self.config['DEFAULT_SPAM_THRESHOLD'] = int(self.config['DEFAULT_SPAM_THRESHOLD'])
This would be the ideal location to add error handling. It is not required. We don't have error handling for any of the other mailu.env environment variables.
Finally in models.py on line 511 you can use now:
spam_threshold = db.Column(db.Integer, nullable=False, default=int(app.config['DEFAULT_SPAM_THRESHOLD]))
The documentation changes looks fine.
This commit adds functionality to set a custom default spam threshold for new users. The environment variable ``DEFAULT_SPAM_THRESHOLD`` can be used for this purpose. When not set, it defaults back to 80%, as the default value was before If ``DEFAULT_SPAM_THRESHOLD`` is set to a value that Python cannot parse as an integer, a ValueError is thrown. There is no error handling for that case built-in.
…onary Per requested changes added the ``DEFAULT_SPAM_THRESHOLD`` to the main application configuration dictionary in ``configuration.py`` and updated ``models.py`` accordingly. No error handling is added, as that was not required.
enginefeeder101
force-pushed
the
configurable-default-spam-threshold
branch
from
1055ccb to
82860d0
Compare
|
Ah I didn't know about the global config dictionary. After I traced the location of the default value, I copied the method used in Email::resolve_destination to retrieve the RECIPIENT_DELIMITER environment variable. That method should probably also be changed to follow the standard configuration tactic. I rebased the commits, due to the added spam_mark_as_read column/variable, and pushed a new commit with the requested changes. Thank you for giving feedback on the pull request. I really appreciate you took the time to explain what needed to be done to be able to be legible to include it in the project. |
|
At a glance it looks good to me. I will test and review the PR this week. bors try |
tryBuild failed: |
|
bors retry |
tryBuild failed: |
Antispam.rst contained a syntax error. Move config description to common section which is more fitting. Fixed wrong assignment of default value for DEFAULT_SPAM_THRESHOLD in models.py.
|
Everything should be fixed. Let's run the test suite bors try |
tryBuild failed: |
|
bors retry |
1 similar comment
|
bors retry |
tryAlready running a review |
tryBuild failed: |
|
bors retry |
tryBuild succeeded: |
|
bors try |
tryBuild succeeded: |
|
bors r+ |
|
Build succeeded:
|
What type of PR?
Feature
What does this PR do?
This PR adds functionality to set a custom default spam threshold
for new users. The environment variable
DEFAULT_SPAM_THRESHOLDisused for this purpose. When not set, it defaults back to 80%, as the
default value was before.
If
DEFAULT_SPAM_THRESHOLDis set to a value that Python cannotparse as an integer, a ValueError is thrown. There is no error handling
for that case built-in. Should that be done?
Prerequisites
Before we can consider review and merge, please make sure the following list is done and checked.
If an entry in not applicable, you can check it or remove it from the list.