feat: added reason column to federation_blocklist #3168

TKilFree · 2023-06-17T14:50:08Z

This allows blocked instances to (optionally) have a reason set.

Addresses issue: #1947

I've just picked this issue up (by searching for 'good first issue's) as a way of dipping my feet into this project and I've only really tinkered with rust in toy projects before so please let me know if there's any problems.

migrations/2023-06-17-132316_add_reason_to_blocklist/up.sql

migrations/2023-06-17-132316_add_reason_to_blocklist/down.sql

dessalines · 2023-06-19T14:37:02Z

migrations/2023-06-17-132316_add_reason_to_blocklist/down.sql

I'm somewhat torn, because nearly every reason we have is in the mod_ tables... the idea being that each action can leave a trace in the modlog. Without modlog tables, those changes in reason, as well as blocking / unblocking instances could get lost.

But I think for this one, its fine, since it'd also be nice to show the reason on the instances page, without having to do any weird sql joins grabbing the most recent reason.

what would ur opinion be on having corresponding mod tables for changes to allow/blocklists, with the source of truth for current values still being the federation_blocklist/federation_allowlist tables? effectively the mod tables would be audit logs rather than something the application itself would refer to

I'd def be good with that. My issue is that I'd rather not duplicate any data, and make sure that the reason is either in one table or the other, but not both. reason typically goes for modlog entries, because things like bans and removes don't necessarily represent a permanent state of affairs. That probably should also apply here, where you might block, then unblock instances.

i've had a quick investigation of this and while it's definitely possible, the changeset did start ballooning because of the joins (to the latest entry of) the new table. i suspect it might be better implemented as a separate commit, but at the end of the day it's up to you whether you'd rather it get done completely and correctly at this point, or whether it's ok to get this in without the log table and i can raise an issue to add it as a separate bit of work?

crates/api_common/src/site.rs

crates/db_schema/src/impls/federation_blocklist.rs

dessalines

Yes its a bit circular, but once @Nutomic approves this, I'll publish a new lemmy-js-client version with these API additions, and ping you.

Then you'll add that version to the api_tests/package.json, and the tests should pass.

crates/db_schema/src/impls/federation_blocklist.rs

api_tests/src/shared.ts

Nutomic · 2023-06-27T09:20:27Z

Looks good to me.

TKilFree · 2023-06-27T14:59:16Z

i've realised that if this was done without using an admin_ table now, it'd potentially be a pain to add one later (since the table would require an admin's id and therefore migrations out of the federation_blocklist table's reason column would be impossible)

just added a commit attempting to solve this using a new admin_block_instance table:

this does lead to some slightly gnarly joins, but i guess we'd expect these tables to remain relatively small?
i've not done the work to actually expose the new admin_ table to the usual modlog stuff, that seems like a chunk of work which could be done in a separate commit?
there's some fiddly logic in federation_blocklist::replace. because we get a full replacement set of blocked instances, then we need to figure out which of these are new instances, which are existing instances but with a different reason and which already existing instances are no longer blocked
i've added tests for the following scenarios:
- going from no blocklist to some blocked instances
- going from some blocked instances to none
- updating the reasons on an existing blocklist
- a backwards compat test for the bit that gets blocked instances to ensure it still works if there are blocked instances without any corresponding admin log entries

dessalines · 2023-07-03T16:00:05Z

migrations/2023-06-27-113917_add_admin_block_instance_table/up.sql

@@ -0,0 +1,10 @@
+-- add the admin_block_instance table
+
+create table admin_block_instance (


Thanks, adding this as a mod table is better.

dessalines · 2023-07-03T16:00:16Z

migrations/2023-06-27-113917_add_admin_block_instance_table/down.sql

@@ -0,0 +1 @@
+-- This file should undo anything in `up.sql`


Missing the table delete.

dessalines · 2023-07-03T16:05:02Z

crates/api_common/src/site.rs

+#[cfg_attr(feature = "full", derive(TS))]
+#[cfg_attr(feature = "full", ts(export))]
+pub struct BlockedInstance {
+  pub id: InstanceId,


Weird... what's this about. It should only be a modlog table now.

dessalines · 2023-07-03T16:05:55Z

crates/api_common/src/site.rs

 #[derive(Debug, Serialize, Deserialize, Clone)]
 #[cfg_attr(feature = "full", derive(TS))]
 #[cfg_attr(feature = "full", ts(export))]
 /// A list of federated instances.
 pub struct FederatedInstances {
  pub linked: Vec<Instance>,
  pub allowed: Vec<Instance>,
-  pub blocked: Vec<Instance>,
+  pub blocked: Vec<BlockedInstance>,


No just return the Instance. The block and reasons should be included in the modlog only.

this is to support the UI being able to show reasons for each blocked domain

dessalines · 2023-07-03T16:07:28Z

crates/db_schema/src/impls/federation_blocklist.rs

+            let mut unblocked_instances = existing_blocked_instances
+              .keys()
+              .cloned()
+              .collect::<HashSet<String>>();


Really weird joining, none of it is necessary. You only need to add the AdminBlockInstance view to the moderation tables.

i think this is necessary unless i'm misunderstanding.

the updates to the blocklist come as a complete set of blocked domains + reasons.
we've then got existing blocked domains + reasons and need to figure out what to put in the mod table, but there can be:

newly added domains;

existing domains with new reasons;

existing unchanged domains;

removed domains.

presumably we want to add entries for any newly added domains or existing domains with new reasons with a blocked flag since they represent admin actions. we also want to add entries for removed domains with the blocked flag false to represent the admin removing that domain.

i think the removed part is definitely necessary, the part figuring out created/edited could be removed, but then if you've got a list of 100 blocked domains and the admin edits the reason for one of them, or just removes one of them you'd be putting 100 unnecessary items in the admin log

Nutomic · 2023-09-28T09:50:19Z

Closing because its abandoned.

TKilFree requested review from Nutomic and dessalines as code owners June 17, 2023 14:50

TKilFree force-pushed the feat-add-reason-to-blocklist branch from 3617c8b to 2a4324e Compare June 17, 2023 15:02

TKilFree mentioned this pull request Jun 18, 2023

feat: provide a reason when blocking instances LemmyNet/lemmy-ui#1367

Closed

dessalines reviewed Jun 19, 2023

View reviewed changes

TKilFree force-pushed the feat-add-reason-to-blocklist branch 8 times, most recently from ea3ecf2 to 6881927 Compare June 21, 2023 10:28

dessalines approved these changes Jun 26, 2023

View reviewed changes

crates/db_schema/src/impls/federation_blocklist.rs Show resolved Hide resolved

api_tests/src/shared.ts Show resolved Hide resolved

dessalines added the breaking change label Jun 26, 2023

Nutomic approved these changes Jun 27, 2023

View reviewed changes

TKilFree force-pushed the feat-add-reason-to-blocklist branch from 6881927 to 81939f4 Compare June 27, 2023 11:34

TKilFree force-pushed the feat-add-reason-to-blocklist branch from 0cec99a to b9f179f Compare June 27, 2023 15:09

TKilFree added 2 commits June 29, 2023 11:34

feat: added reason column to federation_blocklist

ceeaba7

refactor: wip, try adding admin_ table

99c8606

TKilFree force-pushed the feat-add-reason-to-blocklist branch from b9f179f to 99c8606 Compare June 29, 2023 10:35

dessalines reviewed Jul 3, 2023

View reviewed changes

Nutomic closed this Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added reason column to federation_blocklist #3168

feat: added reason column to federation_blocklist #3168

TKilFree commented Jun 17, 2023 •

edited

Loading

dessalines Jun 19, 2023

TKilFree Jun 19, 2023

dessalines Jun 19, 2023

TKilFree Jun 20, 2023

dessalines left a comment

Nutomic commented Jun 27, 2023

TKilFree commented Jun 27, 2023

dessalines Jul 3, 2023

dessalines Jul 3, 2023

dessalines Jul 3, 2023

dessalines Jul 3, 2023

TKilFree Jul 5, 2023

dessalines Jul 3, 2023

TKilFree Jul 5, 2023

Nutomic commented Sep 28, 2023

		@@ -0,0 +1,10 @@
		-- add the admin_block_instance table

		create table admin_block_instance (

feat: added reason column to federation_blocklist #3168

feat: added reason column to federation_blocklist #3168

Conversation

TKilFree commented Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

Nutomic commented Jun 27, 2023

TKilFree commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Sep 28, 2023

TKilFree commented Jun 17, 2023 •

edited

Loading