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.

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

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

Conversation

ianmacd
Copy link
Contributor

@ianmacd ianmacd commented Jan 24, 2023

This PR complements #167, which has already been merged.

Here, we add new endpoints to allow the deletion of a given user's emoji reactions from all messages in a given room, or from every room on the server.

/room/<Room:room>/all/reactions/<SessionID:sid> removes all reactions placed by the given user on messages in the specified room.

/rooms/all/reactions/<SessionID:sid> expands on this by removing all reactions by the given user from all rooms on the server.

As with message deletion, global moderator privileges are required for server-wide deletion.

The code has been tested and verified working on sog.caliban.org.

Care has been taken to incorporate in this pull-request the review feedback given in #167.

`/room/<Room:room>/all/reactions/<SessionID:sid>` removes all reactions
placed by the given user in the specified room.

`/rooms/all/reactions/<SessionID:sid>` expands on this by removing all
reactions by the given user from all rooms on the server.

Global moderator privileges are required for server-wide deletion.
ianmacd added a commit to ianmacd/session-desktop that referenced this pull request Feb 3, 2023
This expands on oxen-io#2653 and ensures that when a user is banned from a room
or an entire server, and **Delete All** has been selected, not just his
messages but also his emoji reactions will be removed.

This pull-request depends on oxen-io/session-pysogs#169.

Verified working on `sog.caliban.org`.
'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

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

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).

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.

total += count
deletions[room.token] = count
except exc.BadPermission:
pass
Copy link
Member

@jagerman jagerman Feb 17, 2023

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.

ianmacd added a commit to ianmacd/session-desktop that referenced this pull request Aug 9, 2023
This expands on oxen-io#2653 and ensures that when a user is banned from a room
or an entire server, and **Delete All** has been selected, not just his
messages but also his emoji reactions will be removed.

This pull-request depends on oxen-io/session-pysogs#169.

Verified working on `sog.caliban.org`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants