Add bulk bulk_action_parent_id field to modlog table#6352
Add bulk bulk_action_parent_id field to modlog table#6352dessalines merged 14 commits intoLemmyNet:mainfrom
Conversation
Closes LemmyNet#6323 Adds a new `bulk` boolean column to the modlog table so that bulk actions (e.g. removing all content when banning a user) can be distinguished from individual mod actions in the frontend.
881d6d8 to
10a8ca0
Compare
|
wouldn't it make more sense to store the parent id instead? non-bulk/parents could have this set to |
I think this makes sense. just to make sure I follow completely, for example The ban entry is inserted into modlog first → gets id = 36 , parent_id = NULL (it's a root action, nobody triggered it) bulk actions are therefore anything where the parent_id is not null One thing is that I'll likely need to ensure this works on the federated path since parent_id requires the parent row to exist first.parent_id requires the parent row to exist first. Thanks for the feedback @Nothing4You let me know if I'm understanding your feedback fully. |
|
I agree that it makes sense to include the parent id, so they can be grouped together in the UI. To be more explicit I would call it It would also make sense to change ModlogView to hide bulk items by default, and then add a new api parameter |
There was a problem hiding this comment.
I don't know if it makes sense to include the parent_id, because we already have modlog filters for post_id, comment_id, community_id.
Because if we did add parent_id, I'd prefer that to reference which type of id it would be (IE parent_post_id, parent_community_id) , and that could get pretty complicated. The filters above should already work good enough, and having a simple is_bulk to GetModlog should be enough.
Another way to put it, is that the modlog table already stores things like target_post_id, target_community_id, etc. So adding another target_parent_id would be redundant.
crates/api/api/src/site/mod_log.rs
Outdated
| ModlogView { | ||
| modlog: Modlog { | ||
| is_revert: true, | ||
| is_bulk: true, |
There was a problem hiding this comment.
All these should also be false.
| .list(pool) | ||
| .await? | ||
| .items; | ||
| assert_eq!(1, modlog.len()); |
There was a problem hiding this comment.
Also be sure to add tests for when bulk removal is true.
It's not redundant to have data that clearly links the two actions. From UI/client perspective, I think the following would be useful:
Additionally, having explicit modlog child records are useful when reversing a ban, by being able to selectively undo e.g. post/comment removals from a community ban. A moderator could click an undo button on a community ban modlog entry and this could restore all posts that were removed as part of the ban, without restoring posts that may have been removed for unrelated reasons. |
It does make sense to link them, but then we'll at least need
I think in that case it makes sense to add the |
the parent would refer to another modlog entry. i'm not quite sure why we would need to split the references by type? a single modlog entry should only have a single parent, and they'd all refer to another row in the same table. |
"a single modlog entry should only have a single parent, and they'd all refer to another row in the same table." this makes the most sense to me. I want to confirm this is the approach we'd like to move forward with before implementing again. I'll call it |
|
That seems fine. I think on the API side, we'll still just need a |
Instead of a boolean flag, store a reference to the parent modlog entry that triggered bulk content removal (e.g. the ban that caused all of a user's posts to be removed). NULL means an individual action; non-null means a bulk action triggered by the referenced entry.
|
You need to format the code with |
thanks . Will check through that now |
| ALTER TABLE modlog | ||
| ADD COLUMN bulk_action_parent_id int REFERENCES modlog (id) ON DELETE SET NULL; | ||
|
|
||
| CREATE INDEX idx_modlog_bulk_action_parent_id ON modlog (bulk_action_parent_id); |
There was a problem hiding this comment.
Can use where bulk_action_parent_id is not null to use less storage space.
There was a problem hiding this comment.
i think over time the majority of rows will probably have this set, so i don't think this will long term be saving a significant amount of space.
it would exclude the index from being usable for non-bulk queries. i'm not sure if would make a noticeable difference in practice. an index scan for published would happen first and then i'd expect a sequential scan to rule out rows that were retrieved from the published index but have the bulk column null. for large bulk operations this could result in hundreds to thousands of rows being read and skipped from the sequential scan.
it might make sense to generate some synthetic data and look at query plans.
|
thanks for the feedback all. Will address it this weekend |
mod_remove_comment constructor functions - Pass Some(id) at bulk call sites (utils.rs) and None at individual mod action sites (api_crud, apub deletion/undo_delete) - Fix ModlogQuery to skip IS NULL guard when bulk_action_parent_id filter is provided (was making the eq filter unreachable)
…odlog # Conflicts: # crates/api/api_crud/src/comment/remove.rs # crates/api/api_crud/src/post/remove.rs # crates/api/api_utils/src/utils.rs # crates/apub/activities/src/deletion/delete.rs # crates/apub/activities/src/deletion/undo_delete.rs # crates/db_views/modlog/src/impls.rs # crates/db_views/notification/src/tests.rs
09b8f09 to
5e4b075
Compare
dessalines
left a comment
There was a problem hiding this comment.
Only 1 major thing having to do with bulk comment removals. Everything else is minor:
…lk_action_parent_id as well. Also moved parent_id into varsper pr feedback
sorry got busy with work. Thanks for all the feedback all! my first contribution to this project. |
This is for issue #6323
each bulk child entry now stores a foreign key back to the modlog row that triggered it