chore: Fix mock ASE seeding script #3050
Conversation
github-actions
bot
added
type: tests
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
| @@ -2,6 +2,14 @@ type Query { | |||
| "Fetch an asset by its ID." | |||
| asset("Unique identifier of the asset." id: String!): Asset | |||
|
|
|||
| "Get an asset based on its currency code and scale if it exists." | |||
| assetByCodeAndScale( | |||
There was a problem hiding this comment.
we should add bruno requests for these queries
There was a problem hiding this comment.
Just thinking out loud, but another option would be to not actually add these resolvers but just use assets and peers queries to fetch all assets & peers. But I think these might be useful to include.
@oana-lolea @BlairCurrey opinions on this?
There was a problem hiding this comment.
An asset code and scale combination is unique, right? Might make sense for the graphql schema to reflect that in some way.
There was a problem hiding this comment.
I thought these queries might be reused in the future, because they are searching by the unique column(s) of the tables. But if not, I can just filter everything in the script and it might be a bit faster than just querying the db that much.
There was a problem hiding this comment.
An asset code and scale combination is unique, right?
Yes, has a unique constraint in DB.
@njlie what do you mean by "Might make sense for the graphql schema to reflect that in some way."?
There was a problem hiding this comment.
What I mean to say is, from the point of view of someone integrating with the Admin GraphQL API and looking at it for the first time, seeing a resolver like assetByCodeAndScale would be an effective way to communicate to that audience that those two fields together is a unique combination
There was a problem hiding this comment.
Ah, I see what you mean. As in, the schema itself reflects to the integrator about how assets work. I thought you meant we needed to add a comment or something to the schema. Understand now.
There was a problem hiding this comment.
I agree in general, let's keep these resolvers. @oana-lolea I think the outstanding item is just to remove some lines in the resolver test file
|
@oana-lolea make sure to go through the PR checklist as well |
| @@ -2,6 +2,14 @@ type Query { | |||
| "Fetch an asset by its ID." | |||
| asset("Unique identifier of the asset." id: String!): Asset | |||
|
|
|||
| "Get an asset based on its currency code and scale if it exists." | |||
| assetByCodeAndScale( | |||
There was a problem hiding this comment.
Just thinking out loud, but another option would be to not actually add these resolvers but just use assets and peers queries to fetch all assets & peers. But I think these might be useful to include.
@oana-lolea @BlairCurrey opinions on this?
Changes proposed in this pull request
Context
When spinning up the local environment and the database volumes were not removed, the mock ASE seeding script tries to create assets, peers and wallet addresses to populate the database, but fails, because the values already exists and the mock user accounts are not created.
By checking if the data already exists by querying by their unique entries, this issue can be fixed. If an entry already exist, the seeding script will use those instead of creating them.
Added #3052 issue for updating the commands in the user docs.
Fixes #2945
Checklist
fixes #numberuser-docslabel (if necessary)