Allow admins to see policy server-flagged events

[turt2live]

Requires #18238 Merged!

Real diff: 881d521...travis/flag-ps-events

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct (run the linters)

[MadLittleMods]

Discussed in the #synapse-dev:matrix.org room

[anoadragon453]

The term "spammy" doesn't feel like a real term to me... Can we just say "marked as spam"?

If you were looking for a short field name, policy_server_spam seems fine.

[turt2live]

sorry for the force push: I thought I set this PR up for a clean merge of develop->PR, but forgot all of the important steps.

[turt2live]

"spammy" comes from the detail that the policy server doesn't technically get final say on whether the event is spam or not, despite that being the current implementation. It's also used in other places throughout the codebase, especially around the existing spam checker interfaces.

I'd prefer to keep it as an invented word, but can change it if needed.

[anoadragon453]

Some more iteration.

Could you also add some unit tests for this new field please?

[anoadragon453]

To match the docs:

Suggested change

	"return_policy_server_spammy_events", False
	"return_policy_server_spammy_events", self.return_soft_failed_events

[anoadragon453]

Is this not supposed to be here?

[anoadragon453]

The logic here is currently quite difficult (for me) to follow. I'd find this easier to read as:

if (
    filter_send_to_client
    and await storage.main.is_server_admin(UserID.from_string(user_id))
):
    if client_config.return_soft_failed_events:
        # Include events that were soft-failed by any means.
        # `events` does not need to be modified in this case.
    else if (
        not client_config.return_soft_failed_events
        and client_config.return_policy_server_spammy_events
    ):
        # Include events that were only soft-failed by a policy server.
        # Events that were soft-failed by other means are excluded.
        events = [
            e
            for e in events
            # Return non-soft-failed events as well as those explicitly marked
            # as spam by a policy server. This excludes events that were soft
            # failed by other means.
            if not e.internal_metadata.is_soft_failed()
            or e.internal_metadata.policy_server_spammy
        ]
    else if (
        not client_config.return_soft_failed_events
        and not client_config.return_policy_server_spammy_events
    ):
        # Only include non-soft-failed events.
        events = [e for e in events if not e.internal_metadata.is_soft_failed()]