Antispam features

[Athozus]

This is WIP. It follows suggestions listed in #102.

Note that it contains many changes, such as :

• Add warning, disabled and muted colors, respectively #FF8800, #332222 and #CCCCCC.
• Enlarge boxes views to have enough space for new (un)mark as spam buttons
• New message view layout for the same reason as before
• Add a list setting type. I "cheated" a bit on an if statement so I will rework it later, but it works without any problem at the moment (to be sure it is completely modular).
• Add a new Spam group to the settings groups, with the new setting Mute list (and a toggle button in message view, not visible yet in the screenshot)

It has a dependency with merge request #127

I will rebase (with some squashes) because it has some commits that are technically independent from the antispam itself.

More details about changes, check commits messages.

[Athozus]

There are some problems with luacheck (I don't really know how to fix it) but the table.indexof feature is available since 5.0.0.

[Niklp09]

(I don't really know how to fix it) but the table.indexof feature is available since 5.0.0.

Luacheck doesn't know some functions since they don't belong to the lua specification.
You must add them manually to the read_globals table (table = {fields = {"indexof"}}).

[Athozus]

Thanks. All is ok for the review ?

[Athozus]

Until I do some fixes for that you can report, I plan to do the following steps :

• Rebase Add support for shared settings #127
• Rebase this PR on master ("remove" Add support for shared settings #127 commits on this PR worktree)
• Squash luacheck fixes
• Squash mute_list and its sharing
• Spare check into add check for settings and implement the function for mute_list itself
• Rebase

[S-S-X]

Disable beerchat syncing until muting gets fixed

API should be fixed when mt-mods/beerchat#116 is merged.
If you want you could test it with upgrade-mute-bugs branch.

[Athozus]

API should be fixed when mt-mods/beerchat#116 is merged. If you want you could test it with upgrade-mute-bugs branch.

It successfully works, thanks ! I git force push to remove the latest commit, because it is soon merged, or I let this fix and re-enable sync when it is merged in beerchat master ?

[BuckarooBanzay]

lgtm if this is properly tested

[BuckarooBanzay]

what are you planning to do with this function, this looks like a cpu-hog 🤔

[Athozus]

check (the end of the part of code) or sync ?

• check is to be sure all usernames given are existing
• sync is to get data from beerchat, but it is temporary disabled waiting for mute fixes merged

[S-S-X]

I do still think that data sync for muting could be a way better as event based specific checks than a static lists and settings sync.

Additionally for beerchat muting to stay sync globally both ways you do not really have to transfer any static data at all and also already has fairly good performance for most parts when used directly.

So making this side also event based would allow for example (if something in this list does not make sense feel free to ask for clarification):

• getting rid of somewhat complicated data transfer between the mods.
• likely better performance without additional optimizations, caching or usage restrictions.
• no loading or syncing of data for players who do not use /mail.
• data only fetched when it is actually needed.
• data always up to date, no synchronization required ever.
• no merging of data and no conflict resolutions for merge conflicts.
• does not have to worry about merging possible offline data updates (external systems for example).
• overall simpler compared to data synchronization.
• automatic handling both public list (other mods) and private list (mail mod), just user interface could use specific code for public/private distinction.

I've not put a lot thought on config sync system so I'd love to hear what problems it can solve and why it would be preferred over on-demand API calls or other event based checks.

Basically just saying that my comments on #127 also apply here.

[Athozus]

Non-using beerchat disables the syncing script. This is on purpose : it syncs list only when the setting is called. I don't want to have mail storage outside mail : transfer is better than putting muting lists in beerchat. If beerchat is deleted, everything is out.

[S-S-X]

If beerchat is deleted, everything is out.

Did I get it right: basically you want that beerchat configrations keep affecting mail even after beerchat has been removed?

[Athozus]

👍 Yes, and it saves resources by syncing only when needed.

[S-S-X]

It actually very likely wastes resources and has worse performance as it syncs what potentially isn't needed but also it is probably simplest way to keep configurations (both wanted and unwanted) after removing source mod.

[Athozus]

It actually very likely wastes resources and has worse performance as it syncs what potentially isn't needed but also it is probably simplest way to keep configurations (both wanted and unwanted) after removing source mod.

If you have a better idea tell me, likely beerchat could have a similar transfer function ?

[S-S-X]

It actually very likely wastes resources and has worse performance as it syncs what potentially isn't needed but also it is probably simplest way to keep configurations (both wanted and unwanted) after removing source mod.

If you have a better idea tell me, likely beerchat could have a similar transfer function ?

See comment in another thread (code comments) https://github.com/mt-mods/mail/pull/128/files#r1451568438

[S-S-X]

This is bad idea as it will likely change to mod storage (because player meta is very limitated).
For this I'd recommend using API as it will then actually keep working after beerchat's internal storage is updated.

[S-S-X]

Additionally it introduces another inconsistency as it depends on /mute command which is not definition of beerchat mute functionality.

[Athozus]

Okay because when I did it it was still the buggy muting API. I'm gonna make a commit in a second.

[Athozus]

EDIT : there's not yet a mute_player API command in plugin/mute.lua

[S-S-X]

Muting API was not buggy for muting ever. Only for those shortcut API functions you can use to check if someone is muted.

Actual muting API uses event based checks, shortcut functions were not actually used anywhere (never called internally or externally) and were broken but those are only shortcuts for checking status. Muting is event based, not direct API call (because there's no single configuration for muting so you cannot store configuration for muting anywhere).

So, you implement check by adding this:

if minetest.get_modpath("beerchat") then
	beerchat.register_callback('before_send_pm', function(name, message, target)
		if mail.get_setting(target, "has_muted_"..name) then
			return false
		end
	end)
end

Then use that check by calling (this shortcut was function buggy before but is now fixed):
this should be added where mail is actually sent

if beerchat.allow_private_message(from, to) then
	mail.do_actually_deliver_mail_message_to_recipient(from, to, body)
end

Note that above code is just to explain basic concept by example, I've no idea about exact mail mod functions / mail mod settings storage but know it is something like this just with different naming / paths.

Note 2 this will automatically mute other things, both ways, like jailed players and teleport requests for muted players.

Note 3 this automatically supports (no additional code besides what I already wrote up there ^^) skipping muting for special cases like server admin or moderators if server owner wants that. Also any other muting behavior is automatically synced without having to write any code.

[Athozus]

Ok for the suggestions (I already read them but didn't have the time to work on).

I can make a commit based on your suggestions, but I'm won't think it would be much better (we still have to get a copy for ourselves).

The idea is not to push to master a buggy PR, however my goal is to have #127 and #128 rebased to have a common base before merging new PRs (I don't want to treat a lot of merging conflicts).

Tell me if you're ok for the rebase. I already did a lot of tests, I fixed what I found, so for me it's stable actually.

[S-S-X]

Comments are based on current PR target branch which currently is master.

Any rebases on development branches are always fine, no issues with that and should be done whenever you feel it helps with development.

But you can always switch that to another dev branch, also rebase on that dev branch if needed and it is actually what I do too when not sure about something, want to compare different somewhat complicated approaches or just want to get easy view to big picture about possible changes.

[Athozus]

Thanks for the answer.

For me, it is now operational (might be some lost bugs, as any software), but there's not so much issues at the moment.

It is a big pull request (gonna be rebased, with some squashes btw), so I would like other maintainers advice before doing it (such as @BuckarooBanzay )

[BuckarooBanzay]

For me, it is now operational (might be some lost bugs, as any software), but there's not so much issues at the moment.

👍

It is a big pull request (gonna be rebased, with some squashes btw), so I would like other maintainers advice before doing it (such as @BuckarooBanzay )

just squashing sounds fine, i would even do that into the master branch IMO (if the code is properly tested)

[Athozus]

That's what I planned to do. I'm starting right now.

[Athozus]

@S-S-X @BuckarooBanzay

I followed what I said, following the suggestion from Buckaroo to squash #127 (I haven't understood when I answered, but after reading again the message it was clear).

Here's my working tree. I want to be sure that it's not polluting master branch, but I'm quite satisfied of I what I did, so I consider one approval so one of you will be ok for rebasing.

[S-S-X]

Don't really want approve as long as direct player metadata reading or writing is included. There's anyway one approval already...

If you believe it wont cause issues when metadata is obsoleted and/or cleaned up then probably fine, just keep in mind that it is completely private data that is not considered being part of beerchat API and should be refactored soon to allow missing features.

Additionally just a reminder, I didn't read code well enough to be sure but it seems to loop over all players and then read/write their metadata. Is there a checks in place to make sure it wont try to do it for offline players, I think no which might cause crash if sync is called while any of the registered players is offline.
edit. Did read a bit more and seems it just grabs names for offline players and wont try to use their meta which should be fine (but unnecessary cpu hog).