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

Sort pubs before limiting #923

Merged
merged 16 commits into from
Feb 10, 2025
Merged

Sort pubs before limiting #923

merged 16 commits into from
Feb 10, 2025

Conversation

kalilsn
Copy link
Member

@kalilsn kalilsn commented Feb 4, 2025

Issue(s) Resolved

#890

High-level Explanation of PR

This PR addresses the confusing sort order of pubs by duplicating the orderBy clause in our pubs query into the "root" cte where we apply the limit and offset, rather than applying the limit and offset before sorting the pubs!

It also adds a trigger that updates a pub's updatedAt whenever it's moved between stages, which will ensure that after a pub is moved on the workflows page it's always still in the top 3 pubs selected from its new stage. Finally I did some consolidation of the move query so I could use it in my test.

@tefkah
Copy link
Member

tefkah commented Feb 4, 2025

ahhh makes total sense! good catch!

@kalilsn kalilsn requested review from tefkah and removed request for tefkah February 4, 2025 14:56

const { createForEachMockedTransaction } = await mockServerCode();

const { getTrx, rollback } = createForEachMockedTransaction();
Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up moving this test to its own file, because it fails if it's run as part of another test. I think it has to do with the automatic rollback but I don't completely understand what's happening. Running the test with it.only or moving it to its own file make it pass, although it seems like that prevents the db from being cleaned up (including if I explicitly call rollback).

I think this test failing in a transaction makes sense, since the timestamps are all set using the transaction start time. But I'm not sure why moving it to its own file or using it.only prevents the transaction from working. Also is it ok to do this and just not clean up?

@@ -165,3 +165,15 @@ export const getStages = ({ communityId, stageId, userId }: CommunityStageProps)
};

export type CommunityStage = AutoReturnType<typeof getStages>["executeTakeFirstOrThrow"];

export const movePub = (pubId: PubsId, stageId: StagesId, trx = db) => {
return autoRevalidate(
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't quite remember the context around this, but just want to make sure it covers what we talked about here a long time ago. I think using the .with didn't delete properly, hence the two calls of

await autoRevalidate(
  trx.deleteFrom("PubsInStages").where("PubsInStages.pubId", "=", pubId)
).execute();
await autoRevalidate(
  trx.insertInto("PubsInStages").values({ pubId, stageId })
).execute();

in updatePub?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem wasn't that the delete doesn't work, but that it doesn't necessarily happen before the insert. And I noticed that happening specifically when you update a pub, which attempts to move the pub even if the stage hasn't changed. So in that case, (pubid,stageid) is the same one being deleted and added and we get an error. I added the on conflict do nothing clause below to handle that, and I haven't noticed any issues with the query in other places! It does successfully remove the pub from the stage it's in, and add a new entry for the stage it's moving to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we actually did what that thread says either, since the pub moved trigger wasn't updated. I don't really want to do it in this PR, but I'm happy to make that change in a followup PR, and at the same time get rid of the PubsInStages table and replace it with a pubs.stage column.

Copy link
Member

Choose a reason for hiding this comment

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

very much pro adding a pubs.stageId column instead!

@kalilsn kalilsn enabled auto-merge (squash) February 10, 2025 15:11
@kalilsn kalilsn merged commit cb45900 into main Feb 10, 2025
6 checks passed
@kalilsn kalilsn deleted the kalilsn/fix-pub-sorting branch February 10, 2025 15:38
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.

4 participants