Skip to content

Update coin description + social links - #429

Merged
dharit-tan merged 3 commits into
mainfrom
rt-desc
Sep 30, 2025
Merged

Update coin description + social links#429
dharit-tan merged 3 commits into
mainfrom
rt-desc

Conversation

@dharit-tan

@dharit-tandharit-tan commented Sep 29, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

POST endpoint to update artist coin description + social links

seems like we forgot to update swagger-v1-full.yaml with /coin get endpoint so adding that too.

rickyrombo
rickyrombo reviewed Sep 29, 2025
View reviewed changes
Comment threadapi/v1_update_coin.go Outdated
raymondjacobson
raymondjacobson approved these changes Sep 29, 2025
View reviewed changes

@raymondjacobsonraymondjacobson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good!

Comment threadapi/v1_update_coin_test.go Outdated
Comment threadapi/server.go
schottra
schottra reviewed Sep 30, 2025
View reviewed changes
Comment threadapi/v1_update_coin.go
Comment threadapi/v1_update_coin.go Outdated
Comment threadapi/v1_update_coin_test.go Outdated
Comment threadapi/v1_update_coin.go Outdated
Comment threadapi/v1_update_coin.go Outdated

row := app.writePool.QueryRow(c.Context(), sql, args)

var result struct {

@schottraschottraSep 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why we are returning all of this? Is the goal to merge it into the client side cache? Mostly concerned because it's not the full fieldset from /v1/coins (here), so we would have to be careful not to just replace the client side object.

@dharit-tandharit-tanSep 30, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

i also was wondering this, maybe better to just not return?
do we have a similar update endpoint whose conventions i can follow?

Comment threadapi/v1_update_coin_test.go Outdated
Comment threadapi/v1_update_coin_test.go Outdated
Comment threadapi/v1_update_coin_test.go

assert.Equal(t, 403, status)
jsonAssert(t, body, map[string]any{
"error": "You do not own this coin",

@schottraschottraSep 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I continue to think that testing the exact content of these messages is brittle and unnecessary. But I might be in the minority. :-)

Comment threadapi/v1_update_coin.go Outdated
Comment threadddl/migrations/0165_artist_coins_updated_at_and_social_links.sql Outdated
rickyrombo
rickyrombo approved these changes Sep 30, 2025
View reviewed changes
Comment threadapi/v1_update_coin.go
args := pgx.NamedArgs{"mint": mint}
hasUpdates := false

if body.Description != "" {

@rickyromborickyromboSep 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is unsettling the description allowed?

@dharit-tan
dharit-tan merged commit b4f511f into mainSep 30, 2025
5 checks passed
@dharit-tan
dharit-tan deleted the rt-desc branch September 30, 2025 18:50
@rickyromborickyrombo mentioned this pull request Aug 4, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rickyromborickyromborickyrombo approved these changes
@faridsalaufaridsalauAwaiting requested review from faridsalau
@raymondjacobsonraymondjacobsonAwaiting requested review from raymondjacobson
@schottraschottraAwaiting requested review from schottra

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dharit-tan@schottra@raymondjacobson@rickyrombo