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

Adding combined person content and person saved tables. #5251

Draft
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

dessalines
Copy link
Member

Context #2444

Must come after #5231

Notes:

  • I've removed the content fetch from GetPersonDetails, into its own paged API action: ListPersonContent. Seemed strange to get a person's details, moderated communities, and then a paged content fetch.
  • I've removed the saved_only option from post and comment views, and created a dedicate API action (ListPersonSaved) to fetch combined saved content (posts and comments).
    • We've decided on removing duplicated individual type fetches where the combined routes are preferred, and there was a lot of unecessary complication with published order (IE when the item was saved, not when it was created), that I was able to remove.

dessalines and others added 30 commits November 26, 2024 09:27
* add pagination cursor

* store timestamp instead of id in cursor (partial)

* Revert "store timestamp instead of id in cursor (partial)"

This reverts commit 89359dd.

* use paginated query builder
- Separating the profile fetch from its combined content fetch.
- Starting to separate saved_only into its own combined view.
Comment on lines 716 to 719
-- person_saved (comment, post)
-- TODO, not sure how to handle changes to post_actions and comment_actions.saved column using @dullbanana's trigger method.
-- Post
CREATE FUNCTION r.person_saved_combined_change_values_post ()
Copy link
Member Author

Choose a reason for hiding this comment

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

@dullbananas the tests are passing, but I couldn't figure out a way to do this using your procedure method, so I used the more verbose AFTER INSERT OR DELETE OR UPDATE OF saved ON post_actions to account for all the changes to the X_actions row.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should work

CREATE PROCEDURE r.create_person_saved_combined_trigger (table_name text)
LANGUAGE plpgsql
AS $a$
BEGIN
    EXECUTE replace($b$ CREATE FUNCTION r.person_saved_combined_change_values_thing ( )
            RETURNS TRIGGER
            LANGUAGE plpgsql
            AS $$
            BEGIN
                IF (TG_OP = 'DELETE') THEN
        DELETE FROM person_saved_combined AS p
        WHERE p.person_id = OLD.person_id
            AND p.thing_id = OLD.thing_id;
    ELSIF (TG_OP = 'INSERT') THEN
        IF NEW.saved IS NOT NULL THEN
            INSERT INTO person_saved_combined (published, person_id, thing_id)
                VALUES (NEW.saved, NEW.person_id, NEW.thing_id);
        END IF;
    ELSIF (TG_OP = 'UPDATE') THEN
        IF NEW.saved IS NOT NULL THEN
            INSERT INTO person_saved_combined (published, person_id, thing_id)
                VALUES (NEW.saved, NEW.person_id, NEW.thing_id);
            -- If saved gets set as null, delete the row
        ELSE
            DELETE FROM person_saved_combined AS p
            WHERE p.person_id = NEW.person_id
                AND p.thing_id = NEW.thing_id;
        END IF;
    END IF;
    RETURN NULL;
            END $$;
    CREATE TRIGGER person_saved_combined
        AFTER INSERT OR DELETE OR UPDATE OF saved ON thing_actions
        FOR EACH ROW
        EXECUTE FUNCTION r.person_saved_combined_change_values_thing ( );
        $b$,
        'thing',
        table_name);
END;
$a$;

CALL r.create_person_saved_combined_trigger ('post');

CALL r.create_person_saved_combined_trigger ('comment');

also, always returning null is incorrect

Copy link
Member Author

Choose a reason for hiding this comment

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

That worked! thx.

-- This one is special, because you use the saved date, not the ordinary published
CREATE TABLE person_saved_combined (
id serial PRIMARY KEY,
published timestamptz NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

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

Then rename this column to make it clear.


Ok(PaginationCursorData(token))
}
}
Copy link
Member

@Nutomic Nutomic Dec 12, 2024

Choose a reason for hiding this comment

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

You should be able to move this logic into helper functions which can be used by all the different types.

#[cfg_attr(feature = "full", derive(DieselNewType, TS))]
#[cfg_attr(feature = "full", ts(export))]
/// The person saved combined id
pub struct PersonSavedCombinedId(i32);
Copy link
Member

Choose a reason for hiding this comment

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

According to the other PR these shouldnt have derive(TS, Serialize, Deserialize) nor ts(export)

@dessalines
Copy link
Member Author

I'd like to do these one at a time, so I'll wait till my previous PR is merged to address everything and mark this as ready for review.

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.

3 participants