New user account type "Channel Relay"

[annando]

There is now a new account type, named "channel relay". This is used to distribute posts based on channel definitions.

This is most likely not the finished version, but is ready to review. Further changes will then be done on subsequent PRs.

This PR also fixes #13814

[MrPetovan]

This looks like a WIP, please update its status when you're really done. 😅

[AndyHee]

@hoergen you make an important point about the danger of more spam, but I think it was been established by now that this reshare feature can has existed for a very long time.

In fairness, this enhancement (if I understand this correctly) gives better control for the amount[!] of content reshared and gives admins better oversight about which account is automating.

Also I would like to point to a potential use case for this "new" account type:
https://friendica.xyz/display/adf174d5-2365-9ad1-24d6-4d1456654662

Having said this there are two issues that I still would like @annando to address:

1. Are you planning to retain the feature that allows admins to disable the reshare function? If not, this would be a step backwards, as we would lose the current function that allows admins to disable account automation and consequently controlling the amounts of resources consumed.
2. Are you planning to extent this work and formalise the other hidden account type that "reshares" post as self, while retaining the admin's option to disable it? This would be an improvement for admins to better monitor levels of automation on their nodes.

[annando]

@AndyHee there is an option in the admin settings to enable this feature. It is enabled by default.

[AndyHee]

That's great! So in the future we can still disable the resharing of this "new" account type?

[annando]

This switch will disable the possibility to create that account type. And additionally it disables the reshare functionality, which is important when you already have got such a user in your system. Also this resharing is enabled for group accounts as well, since in the discussions someone said that it would be nice to have some kind of discussion group that additionally can automatically reshare content.

[AndyHee]

I see. Currently my on node there are trusted users who are resharing automatically. But any other users cannot do this, because I have disabled this possibility again after each trusted user had set up everything.

Will this also still be possible in future?

[annando]

I haven't considered that use case by now, sorry. I have to see, how to do that.

[AndyHee]

I really hope that it would be possible to implement this.

[annando]

@MrPetovan This PR should now be finally ready for review.

[MrPetovan]

What are the insertion and deletion for?

[annando]

We need some data to check this type of search.

[MrPetovan]

I don't understand:

• Data is inserted
• Data is selected
• Data is deleted

What does this tell us?

[annando]

$result is important. It will be false when the syntax of the search is invalid. This can happen for example when you use control characters like -, +, ( or ) in a wrong way.

[MrPetovan]

Ah, you're just testing the syntax? You can do it in a single query like this:

SELECT MATCH (`field`) AGAINST (? IN BOOLEAN MODE) FROM `table` LIMIT 1;

No need for insertion nor deletion.

[annando]

I'm not sure, if the system reacts differently, when there is no data in the target table.

[MrPetovan]

Then pick a table where we expect data!

[annando]

I already changed the code.

[MrPetovan]

Does it work as expected even without data?

[annando]

I tested that yesterday, then I made the changes.

[MrPetovan]

Are you still working on it?

[annando]

I just added stuff and fixed things, since the PR is still open.

[MrPetovan]

If you keep adding stuff while it’s in review, it complicates the review. I’d like to know this is the final version.

[annando]

I have got some pending changes, but my priority is that this PR can be merged as soon as possible, so I will try keeping my hands still here.

[MrPetovan]

Thank you, I appreciate it.

[MrPetovan]

Additionally, the messages.po needs refreshing now.

[MrPetovan]

Is all this necessary after you changed the validity test? Is the check-full-text-search table necessary?

[annando]

Yes. Here we don't check the validity. Here we check, if we have got a match.

[MrPetovan]

A match between what exactly? I'm very confused by this code.

[annando]

This specific function checks if there are any user defined channels that would match to the given search text. This function is called at two places. First it is called for posts that arrived via relay. We don't keep all posts there if not explicitly selected. So we check if the post meets some expectations. One is, that it contains hashtags that users subscribed to. And another check is, that there are user defined channels that would select this post.

[MrPetovan]

So we can perform the search directly on the channels table, what's the use of check-full-text-search besides checking for search string validity that we got rid of?

[annando]

No. We execute this query before we consider to even store the content.

[MrPetovan]

I'm thoroughly lost by now. where is the search string defined? What it is supposed to match against? Why do we need the check-full-text-search table?

[annando]

We need to temporarily store the text of the message to a real table (no temporary tables are allowed) to perform the search. That check-full-text-search has got the process id as primary key, since there can be a lot of parallel access to that table.

[MrPetovan]

Ah, thanks, I understand now. Isn't it easier to perform the search in a regular expression instead? Seems less complicated than inserting a row and then immediately removing it from the database.

[annando]

You can use complex search patterns: https://mariadb.com/kb/en/full-text-index-overview/#in-boolean-mode

This means that I would need to create some parser that emulates that search, just to avoid to insert some data into some table.

[MrPetovan]

Please use a wrapper function for this that uses a callback as an argument.

Synopsis would be something like this:

function matchThrowaway(string $searchtext, callable $callback) {
    $this->db->insert('check-full-text-search', ['pid' => getmypid(), 'searchtext' => $searchtext], Database::INSERT_UPDATE);

    $return = $callback($searchtext);

    $this->db->delete('check-full-text-search', ['pid' => getmypid()]);

    return $return;
}

Usage example from the match() method:

$result = matchThrowaway($searchtext, function () use ($search) {
    return $this->inFulltext($search);
};)

For the getMatchingChannelUsers() method:

$uids = matchThrowaway($searchtext, function (string $searchtext) use ($users, $owner_id, $reshare_id, $language, $tags, $media_type) {
	$uids = [];

	foreach ($this->select(['uid' => array_column($users, 'uid'), 'publish' => true, 'valid' => true]) as $channel) {
		if (in_array($channel->uid, $uids)) {
			continue;
		}
		if (!empty($channel->circle) && ($channel->circle > 0) && !in_array($channel->uid, $uids)) {
			if (!$this->inCircle($channel->circle, $channel->uid, $owner_id) && !$this->inCircle($channel->circle, $channel->uid, $reshare_id)) {
				continue;
			}
		}
		if (!empty($channel->languages) && !in_array($channel->uid, $uids)) {
			if (!in_array($language, $channel->languages)) {
				continue;
			}
		} elseif (!in_array($language, User::getWantedLanguages($channel->uid))) {
			continue;
		}
		if (!empty($channel->includeTags) && !in_array($channel->uid, $uids)) {
			if (!$this->inTaglist($channel->includeTags, $tags)) {
				continue;
			}
		}
		if (!empty($channel->excludeTags) && !in_array($channel->uid, $uids)) {
			if ($this->inTaglist($channel->excludeTags, $tags)) {
				continue;
			}
		}
		if (!empty($channel->mediaType) && !in_array($channel->uid, $uids)) {
			if (!($channel->mediaType & $media_type)) {
				continue;
			}
		}
		if (!empty($channel->fullTextSearch) && !in_array($channel->uid, $uids)) {
			if (!$this->inFulltext($channel->fullTextSearch)) {
				continue;
			}
		}
		$uids[] = $channel->uid;
		$this->logger->debug('Matching channel found.', ['uid' => $channel->uid, 'label' => $channel->label, 'language' => $language, 'tags' => $tags, 'media_type' => $media_type, 'searchtext' => $searchtext]);
	}

	return $uids;
});

I didn't test this code, you may have to tinker with it a little bit but at least it's clearer about the meaning of the insertion and deletion.

[annando]

Hmm ... I don't really see an advantage here. In contrary, for me this decreases the readability.

[MrPetovan]

The advantage is that we group the insert and the delete in a single wrapper function instead of having them independent when there is no additional use for either. The function name makes it clear why we're doing this (a throwaway match) which should clear further confusion about this code like I initially had.

[annando]

Interestingly, my first thought about that function name was "what should that name tell me?". It totally confused me. Thanks to your explanation I now understand what you want to express with that name. Also for me this "function as a variable" thing is always hard to read and harder to reproduce. I always have to have a look at other places where this had been implemented to know how to use it.

I'm currently having some other topics that I'm fighting with, also I'm not in a mood for discussions, so I guess I will postpone further work on this PR and will focus on other topics first.

[MrPetovan]

You're the boss, boss!

[annando]

I'm not the boss. I just currently don't have enough energy. Also I really should sleep for several hours now but my body refuses to fall asleep ...