-
-
Notifications
You must be signed in to change notification settings - Fork 864
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
Feature: Configurable default spam threshold used for new users #2328
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: |
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.
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.
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: |
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.
See in-line comment
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.
See in-line comment
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: |
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.
lgtm
bors try |
tryBuild succeeded: |
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.
lgtm. I tested the functionality works as expected
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.
Lgtm
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_THRESHOLD
isused 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 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.