Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for room-wide and server-wide deletion of a user's emoji reactions. #169

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Add support for room-wide and server-wide deletion of a user's emoji reactions. #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1189bee
Internal support for deleting all emoji reactions by a user from a room.
ianmacd Jan 24, 2023
94d3dd0
Add new endpoints for deleting all emoji reactions placed by a user.
ianmacd Jan 24, 2023
05aa702
Also bind to the host's IPv6 address, if present.
ianmacd Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/uwsgi-sogs-standalone.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ gid = GROUP
plugins = python3,http
processes = 2
enable-threads = true
http = :80
http-socket = [::]:80
mount = /=sogs.web:app
mule = sogs.mule:run
log-4xx = true
Expand Down
43 changes: 43 additions & 0 deletions sogs/model/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,49 @@ def delete_all_posts(self, poster: User, *, deleter: User):

return len(deleted), files_removed

def delete_all_user_reactions(self, poster: User, *, deleter: User):
"""
Delete all emoji reactions placed on messages in the room by `poster`.
`deleter` must be a moderator if not deleting his/her own reactions, and
must be an `admin` if trying to delete all of the posts of another admin.
"""

fail = None
if poster.id != deleter.id and not self.check_moderator(deleter):
fail = "user is not a moderator"
elif self.check_admin(poster) and not self.check_admin(deleter):
fail = "only admins can delete all reactions of another admin"

if fail is not None:
app.logger.warning(
f"Error deleting all reactions by {poster} from {self} by {deleter}: {fail}"
)
raise BadPermission()

with db.transaction():
deleted = [
r[0]
for r in query(
'SELECT reaction FROM user_reactions WHERE "user" = :u AND reaction IN (SELECT id FROM reactions WHERE message IN (SELECT id FROM messages WHERE room = :r))',
r=self.id,
u=poster.id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary to actually select the reactions here since we don't actually do anything other than count them; just doing a single-value SELECT COUNT(*) FROM room_reactions WHERE "user" = :u AND room = :r (see comment below for adding that simplifying view) ought to be faster and easier.

)
]

query(
'DELETE FROM user_reactions WHERE "user" = :u AND reaction IN (SELECT id FROM reactions WHERE message IN (SELECT id FROM messages WHERE room = :r))',
r=self.id,
u=poster.id,
)
Copy link
Member

@jagerman jagerman Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These sorts of queries are messy and are better abstracted away in the SQL definition.

I think this trigger would help:

-- sqlite:
CREATE TRIGGER message_reactions_delete INSTEAD OF DELETE ON message_reactions
FOR EACH ROW
BEGIN
    DELETE FROM user_reactions WHERE reaction = OLD.id AND "user" = OLD."user";
END;

-- postgresql:
CREATE OR REPLACE FUNCTION trigger_message_reactions_delete()
RETURNS TRIGGER LANGUAGE PLPGSQL AS $$BEGIN
    DELETE FROM user_reactions WHERE reaction = OLD.id AND "user" = OLD."user";
    RETURN NULL;
END;$$;
CREATE TRIGGER message_reactions_delete INSTEAD OF DELETE ON message_reactions
FOR EACH ROW
EXECUTE PROCEDURE trigger_message_reactions_delete();

And then the query executed in the code here can just be:

DELETE FROM message_reactions WHERE "user" = :u

This, however, is going to delete all a users reactions, server wide, so we would really only want to use it in the global-reactions-delete case.

See comment below for the room-specific version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we should create another view for these cases where we need both reaction + room.

-- This should work for both sqlite/pgsql:
CREATE VIEW room_reactions AS
SELECT reactions.*, user_reactions."user", user_reactions.at, messages.room
FROM user_reactions
    JOIN reactions ON user_reactions.reaction = reactions.id
    JOIN messages ON reactions.message = messages.id;

-- sqlite:
CREATE TRIGGER room_reactions_delete INSTEAD OF DELETE ON room_reactions
FOR EACH ROW
BEGIN
    DELETE FROM user_reactions WHERE reaction = OLD.id AND "user" = OLD."user";
END;

-- postgresql:
CREATE OR REPLACE FUNCTION trigger_room_reactions_delete()
RETURNS TRIGGER LANGUAGE PLPGSQL AS $$BEGIN
    DELETE FROM user_reactions WHERE reaction = OLD.id AND "user" = OLD."user";
    RETURN NULL;
END;$$;
CREATE TRIGGER room_reactions_delete INSTEAD OF DELETE ON room_reactions
FOR EACH ROW
EXECUTE PROCEDURE trigger_room_reactions_delete();

Then queries that want to be room specific can use this view. So for instance:

DELETE FROM room_reactions WHERE room = :r AND "user" = :u;

should accomplish what we want here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for where to put this:

  • The definitions go into schema.sqlite and schema.pgsql, for new installs.
  • Upgrade code to add the new definitions needs to go into a new migration code in sogs/migration (there's a bunch of code to use an examples there now; reactions.py is probably a simple enough one to copy into a new one. Basically:
    • Add a new file with a def migrate(...) that does the check-or-throw for whether the thing being added already exists and whether we're in check-only mode.
    • For sqlite, the migration additions have to be done as separate conn.execute statements
    • For postgresql it is nicer to use a single multi-query statement to add them exactly as is done in the schema.pgsql
    • the new reaction file gets added into migrations/__init__.py to actually get called on upgrade

Copy link
Member

@jagerman jagerman Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(There are probably a couple other cases in the codebase where using this new view would simplify queries, if you're feeling bold, but don't consider it necessary for this PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also if you're wondering why not just add the room into the message_reactions view: the extra JOIN isn't free and can't be optimized away from the query by the SQL engine just because you don't use the value because a JOIN potentially changes the set of rows that get returned).


# FIXME: send `deleted` to mule

app.logger.debug(
f"Delete all reactions by {poster} from {self}: {len(deleted)} reactions"
)

return len(deleted)

def attachments_size(self):
"""Returns the number and aggregate size of attachments currently stored in this room"""
return query(
Expand Down
55 changes: 55 additions & 0 deletions sogs/routes/rooms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,3 +990,58 @@ def delete_user_posts_from_all_rooms(sid):
pass

return jsonify({"total": total, "rooms": deletions})


@rooms.delete("/room/<Room:room>/all/reactions/<SessionID:sid>")
def delete_all_reactions(room, sid):
"""
Deletes all emoji reactions placed by a user in a room

# URL Parameters

- `sid` — the session id of the user whose reactions are to be deleted

# Return value

An empty json object is returned.

# Error status codes

- 403 Forbidden — if the invoking user does not have access to the room.
- 404 Not Found — if the user we are deleting posts from made no posts in this room.
"""
user = muser.User(session_id=sid, autovivify=False)
deleted = room.delete_all_user_reactions(user, deleter=g.user)
if not deleted:
abort(http.NOT_FOUND)
return jsonify({})


@rooms.delete("/rooms/all/reactions/<SessionID:sid>")
def delete_user_reactions_from_all_rooms(sid):
"""
Deletes all emoji reactions by a given user from all rooms.

# URL Parameters

- `sid` — the session id of the user whose reactions are to be deleted

# Return value

A JSON dict with the keys:

- `total` — The total number of reactions deleted across all rooms.
- `messages` — A dict of room tokens and their deletion counts.
"""
deletions = {}
total = 0
user = muser.User(session_id=sid, autovivify=False)
for room in mroom.get_accessible_rooms(g.user):
try:
count = room.delete_all_user_reactions(user, deleter=g.user)
total += count
deletions[room.token] = count
except exc.BadPermission:
pass
Copy link
Member

@jagerman jagerman Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually now that I look at it, it's probably not worth bothering to use the INSTEAD OF DELETE ON message_reactions trigger here. I'd still add it to the schema (since we're already making the other schema changes) because it could be useful if you're tooling around in the database manually and doesn't cost anything to have it, but splitting this up into two different code paths is probably not worth it.


return jsonify({"total": total, "messages": deletions})