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

Convert getCommunities to TRPC and add additional flags. #8777

Merged
merged 47 commits into from
Aug 12, 2024

Conversation

jnaviask
Copy link
Collaborator

@jnaviask jnaviask commented Aug 6, 2024

Link to Issue

Closes: #8756

Description of Changes

  • Adds TRPC query for GetCommunities() which includes requested flags and pagination.
  • Adds /bulkOffchain data to GetCommunity(), so we can deprecate that route as well.
  • Creates and cleans up Tags schemas, incl making pluralization consistent across libs layer (note that the pluralization is technically non-standard in the model. It should be CommunityTag and Tag, but I did not want to make that extensive of a change here).
  • Created new tag unit test, updated groups and stake lifecycle tests to include filtered queries.
  • Updates refresh-all-memberships script to use TRPC query for communities.

Test Plan

  • Ensure groups lifecycle, stake lifecycle, and tags tests succeed.
  • Ensure refresh-all-memberships properly fetches a complete list of communities with groups.
  • Should have no impact on app functioning, as client still uses legacy/deprecated version of route.

Other Considerations

  • Does not remove the original GET /communities route as the UI changes are not complete.
  • Note that GET requests via TRPC do not support arrays as inputs, so we have chosen to use comma-separated strings for tag_ids as a workaround via the zod preprocessor (see: zod array objects break this trpc/trpc-openapi#391 for details on the specific issue).

@jnaviask jnaviask self-assigned this Aug 7, 2024
@jnaviask jnaviask marked this pull request as ready for review August 7, 2024 17:06
@jnaviask jnaviask requested a review from timolegros August 7, 2024 17:44
@rbennettcw rbennettcw self-requested a review August 7, 2024 17:50
rbennettcw
rbennettcw previously approved these changes Aug 7, 2024
libs/model/src/community/GetCommunities.query.ts Outdated Show resolved Hide resolved
@jnaviask jnaviask marked this pull request as ready for review August 9, 2024 14:55
@jnaviask jnaviask requested a review from mzparacha August 9, 2024 15:00
@mzparacha
Copy link
Contributor

mzparacha commented Aug 9, 2024

  1. Still getting a lot of zod errors breaking the server on /communities list route, the /communities/:id works correctly for me.
image

I tried this in frontend

  const { data: community } = trpc.community.getCommunities.useQuery({
    limit: 5,
    base: ChainBase.Ethereum,
    has_groups: true,
    include_node_info: true
  })
  1. The total pages/results counts is still having off by 1/2 issues
image
  1. The expected type for CommunityTags is {id: $tag_id, name: $tag_name}[] per existing implementations of communities route and frontend
image
  1. Not: Not sure if its related but getting this token balance error in the console
image

Everything else worked for me. Thanks

cc: @jnaviask

@jnaviask
Copy link
Collaborator Author

jnaviask commented Aug 9, 2024

@mzparacha will look into the zod errors re (1). Re (2), 2598 results / limit 5 = 519.6 = 520 pages, seems correct to me. Re (3), we should adjust the client expectations to match the database schema, so we don't need to be rewriting the info on the server, unless it would be a heavy lift on your side. (4) seems completely unrelated.

@mzparacha
Copy link
Contributor

mzparacha commented Aug 9, 2024

re: 1 -- 👍
re: 2 -- mb thanks for explaining
re: 3 -- can we do a simple map like this? -- if not I can try to update its instances in frontend
re: 4 -- 👍

cc: @jnaviask

@jnaviask
Copy link
Collaborator Author

jnaviask commented Aug 9, 2024

@mzparacha we cannot do a simple map because the idea is that the result type matches the entity schema defined in libs/schemas, which is based on the database schema. We could return a different tags object at the top level, but not within the Community object.

@mzparacha
Copy link
Contributor

I think replacing

SELECT "community_id",
                json_agg(json_build_object(
                  'community_id', "community_id",
                  'tag_id', "tag_id",
                  'Tag', json_build_object('id', "CommunityTags->Tag"."id", 'name', "CommunityTags->Tag"."name")
                )) as "CommunityTags"
          FROM "CommunityTags"

with

SELECT "community_id",
                json_agg(json_build_object(
                  'id', "CommunityTags->Tag"."id",
                  'name', "CommunityTags->Tag"."name"
                )) as "CommunityTags"
          FROM "CommunityTags"

will be the simplest to get the desired format. Tried and works

image

cc: @jnaviask

@jnaviask
Copy link
Collaborator Author

jnaviask commented Aug 9, 2024

@mzparacha OK I will update the Community entity to permit this special case of data transformation.

@jnaviask
Copy link
Collaborator Author

jnaviask commented Aug 9, 2024

per DM conversation we will NOT be updating the Community entity/queries to use the legacy Tag format

Copy link
Contributor

@mzparacha mzparacha left a comment

Choose a reason for hiding this comment

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

LGTM -- Thanks for all the changes!

@Rotorsoft
Copy link
Contributor

@jnaviask @timolegros @kurtisassad @rbennettcw - we should discuss the conventions we are establishing in this PR and capture the decisions in a new ADR. Then we can do a final cleanup bucket to make sure these conventions are enforced. For example, we found that the best way to represent timestamps (created_at, updated_at) in the schemas is with:

created_at: z.coerce.date().optional()

but there are still several schemas using variants like:

created_at: true
created_at: z.coerce.date().nullish()
created_at: zDate.nullish(),
created_at: z.coerce.date(),
created_at: z.coerce.date().default(new Date()),
created_at: zDate,
created_at: any;
created_at: Date;

// in utils.ts
export const zDate = z.preprocess(
  (arg) => (typeof arg === 'string' ? new Date(arg) : arg),
  z.date(),
);

We should have just one pattern to represent these dates, primary keys, associations, and nullable values.

Copy link
Contributor

@Rotorsoft Rotorsoft left a comment

Choose a reason for hiding this comment

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

LGTM

@jnaviask jnaviask merged commit 42d8131 into master Aug 12, 2024
9 checks passed
@burtonator
Copy link
Collaborator

@jnaviask @timolegros @kurtisassad @rbennettcw - we should discuss the conventions we are establishing in this PR and capture the decisions in a new ADR. Then we can do a final cleanup bucket to make sure these conventions are enforced. For example, we found that the best way to represent timestamps (created_at, updated_at) in the schemas is with:

Unfortunately, the only thing that works is to re-parse the object on the frontend along with z.coerce...

Otherwise the types are wrong. I don't understand why zod doesn't attempt to re-parse them.

@jnaviask
Copy link
Collaborator Author

@burtonator the underlying issue is that sequelize is internally always coercing Date objects back to String on query before returning them, so we then need to reparse them as Date via z.coerce to return them. Let's continue this discussion on a documentation ticket.

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.

Add filter params and pagination to GET /communities endpoint
7 participants