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

Use resourceful route for message reply #5458

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Jan 2, 2025

Creates a nested reply resource for messages.

Changes reply path from /messages/:id/reply to /messages/:id/reply/new because it's a new reply.

Removes unused POST route. It was previously removed in 40cab84, then restored because it was still in use. I don't think it's in use now.

@AntonKhorev AntonKhorev force-pushed the message-reply-resource branch from 1c3605d to 4bd9549 Compare January 2, 2025 08:53
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

This mostly looks good but it turns out it fails when applied on top of #5457 as the reply route no longer works properly when combined with the change to new.

@AntonKhorev AntonKhorev force-pushed the message-reply-resource branch from 4bd9549 to 0016c6f Compare January 3, 2025 08:13
@AntonKhorev
Copy link
Collaborator Author

Updated for compatibility with #5457.

@AntonKhorev AntonKhorev requested a review from tomhughes January 3, 2025 08:20
@tomhughes tomhughes merged commit 44ec0e0 into openstreetmap:master Jan 3, 2025
22 checks passed
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks.

@AntonKhorev AntonKhorev deleted the message-reply-resource branch January 4, 2025 09:13
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.

2 participants