Skip to content
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

Merged

Conversation

enginefeeder101
Copy link
Contributor

@enginefeeder101 enginefeeder101 commented Apr 26, 2022

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 is
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. 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.

  • In case of feature or enhancement: documentation updated accordingly
  • Unless it's docs or a minor change: add changelog entry file.

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2022

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Apr 26, 2022
@enginefeeder101 enginefeeder101 marked this pull request as ready for review April 26, 2022 20:09
@bors
Copy link
Contributor

bors bot commented Apr 26, 2022

try

Build succeeded:

@enginefeeder101 enginefeeder101 changed the title Configurable default spam threshold used for new users Feature: Configurable default spam threshold used for new users Apr 27, 2022
Diman0
Diman0 previously requested changes Jun 8, 2022
Copy link
Member

@Diman0 Diman0 left a 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.
@enginefeeder101 enginefeeder101 force-pushed the configurable-default-spam-threshold branch from 1055ccb to 82860d0 Compare June 8, 2022 15:21
@mergify mergify bot dismissed Diman0’s stale review June 8, 2022 15:21

Pull request has been modified.

@enginefeeder101
Copy link
Contributor Author

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.

@Diman0
Copy link
Member

Diman0 commented Jul 4, 2022

At a glance it looks good to me. I will test and review the PR this week.

bors try

bors bot added a commit that referenced this pull request Jul 4, 2022
@bors
Copy link
Contributor

bors bot commented Jul 4, 2022

try

Build failed:

@Diman0
Copy link
Member

Diman0 commented Jul 4, 2022

bors retry

bors bot added a commit that referenced this pull request Jul 4, 2022
@bors
Copy link
Contributor

bors bot commented Jul 4, 2022

try

Build failed:

Copy link
Member

@Diman0 Diman0 left a 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

Diman0
Diman0 previously requested changes Jul 6, 2022
Copy link
Member

@Diman0 Diman0 left a 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

@Diman0 Diman0 self-assigned this Aug 17, 2022
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.
@ghostwheel42 ghostwheel42 added the type/enhancement Enhances existing functionality label Aug 19, 2022
@mergify mergify bot dismissed Diman0’s stale review August 19, 2022 18:02

Pull request has been modified.

@Diman0
Copy link
Member

Diman0 commented Aug 19, 2022

Everything should be fixed. Let's run the test suite

bors try

bors bot added a commit that referenced this pull request Aug 19, 2022
@bors
Copy link
Contributor

bors bot commented Aug 19, 2022

try

Build failed:

@Diman0
Copy link
Member

Diman0 commented Aug 19, 2022

bors retry

1 similar comment
@Diman0
Copy link
Member

Diman0 commented Aug 19, 2022

bors retry

@bors
Copy link
Contributor

bors bot commented Aug 19, 2022

try

Already running a review

bors bot added a commit that referenced this pull request Aug 19, 2022
@bors
Copy link
Contributor

bors bot commented Aug 19, 2022

try

Build failed:

@Diman0
Copy link
Member

Diman0 commented Aug 19, 2022

bors retry

bors bot added a commit that referenced this pull request Aug 19, 2022
@bors
Copy link
Contributor

bors bot commented Aug 19, 2022

try

Build succeeded:

Diman0
Diman0 previously approved these changes Aug 19, 2022
Copy link
Member

@Diman0 Diman0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mergify mergify bot dismissed Diman0’s stale review October 29, 2022 09:12

Pull request has been modified.

@Diman0
Copy link
Member

Diman0 commented Oct 29, 2022

bors try

bors bot added a commit that referenced this pull request Oct 29, 2022
@bors
Copy link
Contributor

bors bot commented Oct 29, 2022

try

Build succeeded:

Copy link
Member

@Diman0 Diman0 left a 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

Copy link
Contributor

@ghostwheel42 ghostwheel42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@mergify
Copy link
Contributor

mergify bot commented Oct 29, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 29, 2022

Build succeeded:

  • CI-Done

@bors bors bot merged commit 12480cc into Mailu:master Oct 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement Enhances existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants