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 address to results #10362

Merged
merged 4 commits into from
Dec 24, 2024
Merged

Conversation

Rotorsoft
Copy link
Contributor

@Rotorsoft Rotorsoft commented Dec 18, 2024

Link to Issue

Closes: #10356

Description of Changes

  • Fixes query
  • Adds unit test

"How We Fixed It"

Test Plan

  • Unit tested the FIXME() call.
  • CA (click around) tested on local and frack:
    • TODO page

Deployment Plan

  1. @timolegros trigger API client release

Other Considerations

@Rotorsoft
Copy link
Contributor Author

Rotorsoft commented Dec 18, 2024

Replaced sequelize query with plain sql and replaced output schema with CommentsView

@timolegros
Copy link
Collaborator

Why do we need to modify the query? Passing include_user seems to work to return the addresses. include_reactions fails with 500 error but adding duplicating: true on line 41 and 46 of GetComments.query.ts seems to resolve the error (see this Sequelize issue).

@Rotorsoft
Copy link
Contributor Author

we are refactoring queries to use plain sql and return "views".... also note that this query is not used by the app, just the external api

@timolegros
Copy link
Collaborator

timolegros commented Dec 20, 2024

we are refactoring queries to use plain sql and return "views"

We are using view where necessary. I wasn't aware we had decided to convert everything to raw SQL. In this case the Sequelize query is much cleaner IMO.

also note that this query is not used by the app, just the external API

Right so all the more reason to not remove arguments...
Malik is going to use this route to replace existing /viewComments stuff and to paginate comment fetching on the client.

@Rotorsoft
Copy link
Contributor Author

ok, then we can close this without merging and create another PR to fix the query

@timolegros
Copy link
Collaborator

ok, then we can close this without merging and create another PR to fix the query

I'm fine with going with raw query I just hadn't realized we had made that an explicit goal. That said, if we go with raw query we should probably add include_user back.

@Rotorsoft
Copy link
Contributor Author

ok, then we can close this without merging and create another PR to fix the query

I'm fine with going with raw query I just hadn't realized we had made that an explicit goal. That said, if we go with raw query we should probably add include_user back.

I'm ok with both, but prefer raw queries to have more control over model... sequelize is not bullet proof

I don't see the scenario where you want a comment without the user details included, and the performance hit is minimal

@Rotorsoft Rotorsoft requested a review from dillchen December 20, 2024 19:18
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.

Getting some zod errors, but the query works. I have a related PR #10441 where I can get these zod issues fixed.

@Rotorsoft Rotorsoft merged commit 64094d5 into master Dec 24, 2024
10 checks passed
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.

API: add address to reactions, comments, threads field
3 participants